Re: [PATCH 2/2] Test case reproducing PR28030 bug

[Kevin Buettner <kevinb@redhat.com>]

On Wed, 1 Sep 2021 09:35:09 -0400
Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:

> Thanks for the test case.  The comments are very nice, it would be very
> hard to understand without them.  They also communicate the intentions
> well, that will be useful if someone needs to debug it in the future.
> 
> >  gdb/testsuite/gdb.dwarf2/locexpr-dml-lib.c  |  48 +++
> >  gdb/testsuite/gdb.dwarf2/locexpr-dml-main.c |  27 ++
> >  gdb/testsuite/gdb.dwarf2/locexpr-dml.exp    | 360 ++++++++++++++++++++
> >  gdb/testsuite/gdb.dwarf2/locexpr-dml.h      |  30 ++  
> 
> What does "dml" stand for?

data member location, since the use of DW_AT_data_member_location was
one (of several) key elements needed to reproduce the bug.

I'll make a note of the meaning of dml early in the file. If someone
can think of a better name for this test case, I'll use that instead.

> > +# 1) A location expression needed to be used with
> > +#    DW_AT_data_member_location rather than a simple offset.
> > +#    Moreover, this location expression needed to use opcodes
> > +#    which GDB's DWARF reader could not convert to a simple
> > +#    offset.  (Note, however, that GDB could probably be improved
> > +#    to handle the opcodes chosen for this test.)  
> 
> Just to understand what is in parenthesis here: you are saying that GDB
> could be improved to know how to convert the opcodes chosen for this
> test to a simple offset, which I understand would make the test not
> exercise what we want?

That's correct.  The comment in the test case explains it - see
the part about DW_OP_pick and DW_OP_drop.

	# While there are easier / better ways to specify an
	# offset used by DW_AT_data_member_location than that
	# used below, we need a location expression here in
	# order to reproduce the bug.  Moreover, this location
	# expression needs to use opcodes that aren't handled
	# by decode_locdesc() in dwarf2/read.c; if we use
	# opcodes that _are_ handled by that function, the
	# location expression will be converted into a simple
	# offset - which will then (again) not reproduce the
	# bug.  At the time that this test was written,
	# neither DW_OP_pick nor DW_OP_drop were being handled
	# by decode_locdesc(); this is why those opcodes were
	# chosen.

[...]

> > +# Compile the shared library: -DIS_SHAREDLIB prevents main() from
> > +# being defined.  Note that debugging symbols will be present for
> > +# this compilation.
> > +if {[gdb_compile_shlib $libsrc $lib_so \
> > +                       {additional_flags=-DIS_SHAREDLIB debug}] != ""} {
> > +    untested "failed to compile shared library"
> > +    return
> > +}  
> 
> I don't understand when $libsrc is compiled without -DIS_SHAREDLIB.

I call get_func_info in this test case.  It calls function_range. 
which in turn calls gdb_compile.  It's compiled without -DIS_SHAREDLIB
whenever get_func_info is called.

I'll find some place to mention this fact in one of the comments.

> > +
> > +# Compile and start GDB on the main program, again with debugging
> > +# symbols.  Note the use of "shlib=" which indicates that our shared
> > +# library should be used.
> > +if [prepare_for_testing "failed to prepare" ${testfile} \
> > +                        ${srcfile2} [list debug shlib=$lib_so]] {
> > +    return -1
> > +}
> > +
> > +# Do whatever is necessary to make sure that the shared library is
> > +# loaded for remote targets.
> > +gdb_load_shlib ${lib_so}  
> 
> You could save a few lines / cycles by simply loading the .so in GDB to
> get the sizes.  You don't need a running program:
> 
> 
>     $ ./gdb -nx -q --data-directory=data-directory testsuite/outputs/gdb.dwarf2/locexpr-dml/locexpr-dml-lib.so -ex "print sizeof(A)"
>     Reading symbols from testsuite/outputs/gdb.dwarf2/locexpr-dml/locexpr-dml-lib.so...
>     $1 = 16
> 
> I tried, and this works:
> 
>     if {[gdb_compile_shlib $libsrc $lib_so \
>                            {additional_flags=-DIS_SHAREDLIB debug}] != ""} {
>         untested "failed to compile shared library"
>         return
>     }
> 
>     clean_restart $lib_so
> 
>     # Using our running GDB session, determine sizes of several types.
>     set long_size [get_sizeof "long" -1]
>     set addr_size [get_sizeof "void *" -1]
>     set struct_A_size [get_sizeof "g_A" -1]
>     set struct_B_size [get_sizeof "g_B" -1]

I'll make those changes.

> > +
> > +
> > +if ![runto_main] then {
> > +    fail "can't run to main"
> > +    return
> > +}
> > +
> > +# Using our running GDB session, determine sizes of several types.
> > +set long_size [get_sizeof "long" -1]
> > +set addr_size [get_sizeof "void *" -1]
> > +set struct_A_size [get_sizeof "g_A" -1]
> > +set struct_B_size [get_sizeof "g_B" -1]
> > +
> > +if { $long_size == -1 || $addr_size == -1 \
> > +     || $struct_A_size == -1 || $struct_B_size == -1} {
> > +    # No point going on if we can't determine type sizes.
> > +    return  
> 
> Will we get a FAIL or some other indication that something went wrong if
> that happens?

I'll add an error message.

> > +}
> > +
> > +# Retrieve struct offset of MBR in struct TP
> > +proc get_offsetof { tp mbr } {
> > +    return [get_integer_valueof "&((${tp} *) 0)->${mbr}" -1]
> > +}
> > +
> > +# Use running GDB session to get struct offsets
> > +set A_a [get_offsetof A a]
> > +set A_x [get_offsetof A x]
> > +set B_a [get_offsetof B a]
> > +set B_b [get_offsetof B b]
> > +set B_x2 [get_offsetof B x2]  
> 
> I checked, this works too with just loading the .so in GDB and not
> running the program.

Okay.

> > +
> > +# Create the DWARF.  
> > +Dwarf::assemble ${asm_file} {
> > +    global srcdir subdir srcfile srcfile2
> > +    global long_size addr_size struct_A_size struct_B_size
> > +    global A_a A_x B_a B_b B_x2  
> 
> I now adopted the ${::foo} notation to refer to globals.  It spares some
> lines of (possibly unused) global declarations, and I think it makes the
> code more readable, since the :: tells you it's a global at the point
> the variable is used.  I would encourage you to use it as well.

Cool.  I'll do that.

> > +# We don't want a clean restart here since that will be too clean. 
> > +# The original reproducer for PR28030 set a breakpoint in the shared
> > +# library and then restarted via "run".  The command below does roughly
> > +# the same thing.  It's at this step that an internal error would
> > +# occur for PR28030.
> > +runto "bar"  
> 
> Since this runto is what tells us if things worked or not, perhaps pass
> "message" as an option, so that it produces a PASS:
> 
>   runto "bar" message

Sounds good.  I'll make that change too.

Thanks,

Kevin