On Mon, 30 Oct 2017 12:27:15 +0100, Yao Qi wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> > Therefore on old kernels (see below) it will now rather report watchpoint
> > which was hit only next to it (for rwatch/awatch).  But it will never miss
> > a watchpoint.
> 
> I am fine with the change.  Does your case watchpoint-unaligned.c cover
> this?  We need to have a case that show GDB misses a watchpoint (false
> negative), and with your patch applied, it becomes false positive.
> IIUC, all these change happen on the old kernel.

It is handled by the new testcase:

                    # We do not know if we run on a fixed kernel or not.
                    # Report XFAIL only in the FAIL case.
                    if {$expect_hit == 0 && $rdstart < $wpendaligned} {
                        setup_xfail external/20207 "aarch64-*-*"
                    }
                    fail $test

Sure an improvement of the testcase would be to make GDB display its internal
'have_any_contiguous' variable by some maintenance/info/debug command so that
it could display even XPASS for example.  But I do not think the testcase
could be mistaken by that.


> We need a NEWS entry, since the change is user-visible.

Done.


> > There may be a regresion that it now less merges watchpoints so that with
> > multiple overlapping watchpoints it may run out of the 4 hardware watchpoint
> > registers.  But as discussed in the original thread GDB needs some generic
> 
> Is it a regression in your mind or a regression in some existing test
> case?

In my mind.

> If we don't have case, can you write one case to show that, we'll
> run out of watchpoint registers with this fix, but don't run out of them
> before.  We can kfail the failure, which can be addressed by watchpoint
> merging stuff as you mentioned later.

Extended the attached *.exp testfile and filed for it tracker:
	aarch64: watchpoints are not merged regression
	https://sourceware.org/bugzilla/show_bug.cgi?id=22389

Therefore with current FSF GDB it does KPASS.


> Can you elaborate?  watchpoint is quite target specific, and each
> cpu/arch has completely different watchpoint registers.  What do you
> expect watchpoint merging framework do? in a target independent way, I suppose?

Some limitations are specific to arches but most of the logic would be the
same.  There could be some generic merging engine with some arch-specific
parameters.  Such as adjacent watchpoints (watch, not rwatch/awatch) can be
merged into a single larger watchpoint.  Or one could even place there one
bigger watchpoints with possible false positives etc.

But I am not going to implement that.  I consider it only as an incremental
improvement (although it would prevent the regression PR 22389).

I (also) find the code messy but I would have to rewrite it first as there are
member variables instead of accessors, it is not C++, there is missing the
arch-agnostic watchpoint splitting+merging etc.  I coded the patch driven by
the testcase given it tests all the combinations (it does not test splitting of
the watchpoints which I believe is simple enough but maybe that could also be
tested).


> >  aarch64_watchpoint_length (unsigned int ctrl)
> 
> The comments to this function should be updated too.

Done.


> Could you split the patch, that is, patch 1 can be about changing
> aarch64_watchpoint_length and adding aarch64_watchpoint_offset?  That is
> helpful to me to understand the patch.

Sorry I was looking at it for a few evenings and I do not really understand how
you wish the split to be done.  Should it just add the
aarch64_watchpoint_offset function still not supporting the new kernel feature?
Then what it will do there?  It will just always return 0.
If it already should support the new kernels then all the patch needs to be there.
Maybe except aarch64_downgrade_regs but that is a separate function anyway.



> > @@ -148,6 +148,7 @@ struct aarch64_debug_reg_state
> >  
> >    /* hardware watchpoint */
> >    CORE_ADDR dr_addr_wp[AARCH64_HWP_MAX_NUM];
> > +  CORE_ADDR dr_addr_orig_wp[AARCH64_HWP_MAX_NUM];
> 
> Could you add comments on how these two fields are used?

Done.
  // Address aligned down to AARCH64_HWP_ALIGNMENT.
  CORE_ADDR dr_addr_wp[AARCH64_HWP_MAX_NUM];
  // Address as entered by user without any alignment.
  CORE_ADDR dr_addr_orig_wp[AARCH64_HWP_MAX_NUM];


> > +if {![is_aarch64_target] && ![istarget "x86_64-*-*"] && ![istarget i?86-*]} {
> > +    verbose "Skipping ${gdb_test_file_name}."
> > +    return
> > +}
> 
> Any reason to only run this test only on these targets?  Better to run
> this test on every target if possible.

OK.


Thanks,
Jan