RE: [PATCH 2/3] LAM: Enable tagged pointer support for watchpoints.

["Schimpe, Christina" <christina.schimpe@intel.com>]

> > /* Adjust watchpoint address based on the tagging mode which is currently
> >    enabled.  For amd64, we adjust the watchpoint address based on the
> >    currently active linear address masking (LAM) mode using the untag
> >    mask.  Check each time for a new mask, as LAM is enabled at runtime.
> >    Also, the LAM configuration may change when entering an enclave.
> > */
> 
> I still find that a bit long and cumbersome. Can we merge the first and second
> sentence somehow? To me they say the same thing with varying level of details.
> And I don't think we need to say "for amd64", we are in an
> amd64 file.
> 
> And I would drop the enclaves sentence, from GDB perspective it is just another
> type of change at runtime.
> 

Ok, thanks. Will write it as follows then:

Adjust watchpoint address based on the currently active linear address masking
(LAM) mode using the untag mask.  Check each time for a new mask, as LAM is
enabled at runtime.

> > > > +gdb_breakpoint [gdb_get_line_number "Breakpoint here"]
> > > > +gdb_continue_to_breakpoint "Breakpoint here"
> > > > +
> > > > +# Test hw watchpoint for tagged and untagged address with hit on
> > > > +untagged # and tagged adress.
> > > > +foreach symbol {"pi" "pi_tagged"} {
> > > > +    gdb_test "watch *${symbol}"
> > > > +    gdb_test "continue" \
> > > > +	"Continuing\\..*Hardware watchpoint \[0-9\]+.*" \
> > > > +	"run until watchpoint on ${symbol}"
> > > > +    gdb_test "continue" \
> > > > +	"Continuing\\..*Hardware watchpoint \[0-9\]+.*" \
> > > > +	"run until watchpoint on ${symbol}, 2nd hit"
> > > > +    delete_breakpoints
> > > > +}
> > >
> > > I would add a short comment on why we test hitting the watchpoint twice.
> >
> > Hm, I am not sure which information you exactly miss here.
> > Note, that this code is very similar to the code in
> > "gdb.arch/aarch64-tagged- pointer.exp"
> > They have a longer comment:
> >
> > # sp1 and p1 are untagged pointers, but sp2 and p2 are tagged pointers.
> > # Cycle through all of them to make sure the following combinations work:
> > #
> > # hw watch on untagged address, hit on untagged address.
> > # hw watch on tagged address, hit on untagged address.
> > # hw watch on untagged address, hit on tagged address.
> > # hw watch on tagged address, hit on tagged address.
> >
> > But I would have hoped that this is covered by the existent comment:
> >
> > # Test hw watchpoint for tagged and untagged address with hit on
> > untagged # and tagged address.
> >
> > What exactly do you miss?
> 
> I missed that your comment is supposed to cover that.
> You could add an "each", e.g. something like "with a hit on a tagged and
> untagged address each". I think that would already help a lot.

Ok, I will improve the comment as follows then:

Test hw watchpoint for a tagged and an untagged address with hit on a tagged
and an untagged address each.

> And should it be addresses (plural)?

Mh, I don't think so. I added "a" and "an", please see my example above.

Christina 
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928