Re: [PATCH] Support for DW_AT_loclists_base and DW_FORM_loclistx.

["Ali Tamur via gdb-patches" <gdb-patches@sourceware.org>]

Hi Nikita,

Thank you for this patch. I think you have a bug. (At least the version I
have seems to have a bug)

static CORE_ADDR
read_loclist_index (struct dwarf2_cu *cu, ULONGEST loclist_index)
{
...
  read_loclist_header (&header, section);   // (*)
}

I think (*) would be correct only when there is a single location list
header in the .debug_loclists section. In the more general case where there
are many, info_ptr should start reading the header at:
section->buffer + loclist_base - (LOCLIST_HEADER_SIZE32
or LOCLIST_HEADER_SIZE64);

Am I missing something?

My example executable is:
int main() {
  return 17;
}
compiled with clang++, -gdwarf-5 and no split dwarf but also linked with
libc++ library. With your code, gdb cannot read debug_info; with the
correction, everything seems fine.

I have another suggestion/question. Is DW_AT_location allowed to have a
DW_FORM_loclistx form? In the dwarf-dump of my example, I see:
0x00032ee5: DW_TAG_formal_parameter
            DW_AT_location      (indexed (0x85c) loclist = 0x000109d5:
              [0x000000000033b5c0, 0x000000000033b5d0): DW_OP_reg5 RDI
              [0x000000000033b5d0, 0x000000000033b62b): DW_OP_reg15 R15
              [0x000000000033b631, 0x000000000033b65e): DW_OP_reg15 R15)
              DW_AT_name       ("ptr")
              DW_AT_decl_file  ("/path/to/tcmalloc.cc")
              DW_AT_decl_line  (1385)
              DW_AT_type       (0x0003d6d6 "*")

But then:
partial_die_info::read (  ...
  switch (attr.name) case DW_AT_location:
      ...
      else if (attr_form_is_section_offset (&attr)) {
        dwarf2_complex_location_expr_complaint ();  // ***

*** fires, I think needlessly. Could this complaint be obsolete?

Thank you,
Ali


On Fri, Jan 31, 2020 at 12:55 AM Achra, Nitika <Nitika.Achra@amd.com> wrote:

> [AMD Official Use Only - Internal Distribution Only]
>
> Hello Tom,
>
> Thanks for the review. I have incorporated the review comments. Please
> have a look.
>
> Nitika> *Support for DW_AT_loclists_base and DW_FORM_loclistx.
>
> Tom>Thanks for the patch.
>
> Tom>My comments below are primarily nits.
>
> Nitika> Tested by running the testsuite before and after the patch and
> Nitika> there is no increase in the number of test cases that fails.
> Nitika> Tested with both
> Nitika> -gdwarf-4 and -gdwarf-5 flags. Also tested -gslit-dwarf along
> Nitika> with
> Nitika> -gdwarf-4 as well as -gdwarf5 flags.
>
> Tom> I assume it fixed some tests with -gdwarf-5?  Or else we'll need a
> new test case.
>
> Added a new test case. This test checks if the file command passes without
> any error with -g-dwarf-5 and -gsplit-dwarf.
> Gcc emits DW_FORM_loclistx only with -gsplit-dwarf. I can also check by
> printing the variable value, but right now printing
> the value is throwing  the error- " DWARF ERROR: Corrupted dwarf
> expression"  which is not related to this patch.  I have submitted
> another patch which will fix that.
>
> Nitika> +/* Size of .debug_loclist section header for 32-bit DWARF
> Nitika> +format. */ #define LOCLIST_HEADER_SIZE32 12;
> Nitika> +
> Nitika> +/* Size of .debug_loclist section header for 64-bit DWARF
> Nitika> +format. */ #define LOCLIST_HEADER_SIZE64 20;
>
> Tom> The ";" at the end of these defines is weird.
> Removed
>
> Nitika> +  /* Header data from the location list section. */  struct
> Nitika> + loclist_header* loclist_header;
>
> Tom> gdb style puts the "*" on the other side of the " " like
>
> Tom> struct loclist_header *loclist_header;
>
> Nitika> +static struct dwarf2_section_info* cu_debug_loc_section (struct
> Nitika> +dwarf2_cu* cu);
>
> Tom> Here too -- there are actually several instances in the patch.
>
> Done
>
> Nitika> +void
> Nitika> +read_loclist_header (struct dwarf2_cu* cu, struct
> Nitika> +dwarf2_section_info* section) {
>
> Tom> New functions should have an introductory comment explaining their
> purpose.
> Done
>
> Tom> The "static" should be repeated here, rather than rely on the
> declaration.  This affects all of the new functions, I think.  Also,
> there's no need to forward Tom>declare them if the uses come after the
> definition, so probably some of those declarations can be removed as well.
> Tom>Added static in definitions also.
>
> Done
>
> Tom>It might be nicer if read_loclist_header took a pointer to the
> loclist_header rather than a dwarf2_cu, and did not allocate.  See below.
>
> Done
>
> Nitika> +ULONGEST
> Nitika> +lookup_loclist_base (struct dwarf2_cu* cu) {
> Nitika> +  /* For the .dwo unit, the loclist_base points to the first
> offset following
> Nitika> +     the header. The header consists of the following entities-
> Nitika> +     1. Unit Length (4 bytes for 32 bit DWARF format, and 12
> bytes for the 64 bit format)
> Nitika> +     2. version (2 bytes)
> Nitika> +     3. address size (1 byte)
> Nitika> +     4. segment selector size (1 byte)
> Nitika> +     5. offset entry count (4 bytes)
> Nitika> +     These sizes are derived as per the DWARFv5 standard. */
> Nitika> +  if (cu->dwo_unit)
> Nitika> +  {
> Nitika> +    if (cu->header.initial_length_size == 4)
> Nitika> +      return LOCLIST_HEADER_SIZE32;
> Nitika> +    return LOCLIST_HEADER_SIZE64;
>
> Tom>Is there some way to avoid hard-coding sizes here?
>
> I thought of using sizeof(struct loclist_header) in place of
> LOCLIST_HEADER_SIZE32 and sizeof(struct loclist_header) +
> cu->initial_length_size in place of
> LOCLIST_HEADER_SIZE_64. But I am not sure if this a correct way of doing
> this. Some compilers append some padding at the end of structure. So the
> size of structure might not be equal to the sum of size of its members.
> Sizeof() is also compiler dependent. So, right now I cannot think of any
> other way.
>
> Nitika> +/* Given a DW_FORM_loclistx value loclist_index, fetch the offset
> from the array
> Nitika> +   of offsets in the .debug_loclists section. */ CORE_ADDR
> Nitika> +read_loclist_index (struct dwarf2_cu* cu, ULONGEST
> Nitika> +loclist_index) {
> ...
> Nitika> +  const gdb_byte* info_ptr;
>
> Tom> This can be declared later, when it's first initialized.
> Done
>
> Nitika> +  if (section->buffer == NULL)
> Nitika> +    error(_("DW_FORM_loclistx used without .debug_loclist section
> [in module %s]"),
> Nitika> +       objfile_name (objfile));
>
> Tom>I wonder whether errors here will really do something good.  The
> problem is that the DWARF reader, in general, doesn't handle errors very
> well.
> Tom>It *should* -- but it doesn't.  I don't know about this spot, but in
> other places, calling error will mean that reading all of the debuginfo for
> the entire file Tom>will be aborted.  (It can even cause worse problems,
> there's a bug in bugzilla about it ending a remote session.)
>
> Tom>Maybe complaint() and then a fallback would be preferable.
> Tom>Or, test the error() to make sure it's ok.
>
> Replaced with complaint()
>
> Nitika> +  delete cu->loclist_header;
>
> Tom>This is created, only to be deleted in the same function.
> Tom>I think it would be better to just stack-allocate this.
>
> Done
>
> Tom>Or, should this be cached in the CU -- that is, read once and then
> reused?
> Tom>If so then a different approach should be used.  It wasn't clear to me
> how often read_loclist_index is called.
>
> Nitika> +    case DW_FORM_loclistx:
> Nitika> +    {
> Nitika> +      *need_reprocess = true;
> Nitika> +      DW_UNSND(attr) = read_unsigned_leb128 (abfd, info_ptr,
> Nitika> + &bytes_read);
>
> Tom>Space before the first "(".
> Done
>
> Regards,
> Nitika Achra
>