[PATCH] Support for DW_AT_loclists_base and DW_FORM_loclistx.

Mon Feb 24 08:48:00 GMT 2020

[AMD Official Use Only - Internal Distribution Only]

Hi Tom,
Hi Ali,

Sorry for the delayed response. I was in a very poor internet connectivity area. Please look for my comments below.

From: Ali Tamur <tamur@google.com>
Sent: Sunday, February 2, 2020 8:40 AM
To: Achra, Nitika <Nitika.Achra@amd.com>
Cc: Tom Tromey <tom@tromey.com>; gdb-patches@sourceware.org; George, Jini Susan <JiniSusan.George@amd.com>
Subject: Re: [PATCH] Support for DW_AT_loclists_base and DW_FORM_loclistx.

Ali>Hi Nikita,

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

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

Ali>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, Ali>info_ptr should start reading the header at:
Ali>section->buffer + loclist_base - (LOCLIST_HEADER_SIZE32 or LOCLIST_HEADER_SIZE64);

Ali>Am I missing something?

Ali>My example executable is:
Ali>int main() {
Ali> return 17;
Ali>}
Ali>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, Ali>everything seems fine.

I am not able to reproduce this bug using the example executable. The example is working fine with me. I have used the following command-
clang++ -gdwarf-5 -O3  -stdlib=libc++  test.cpp
Could you please look into this again? Am I doing something wrong here?
I have used clang version 10.0.0.

Ali>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:
Ali>0x00032ee5: DW_TAG_formal_parameter
Ali>    DW_AT_location      (indexed (0x85c) loclist = 0x000109d5:
Ali>     [0x000000000033b5c0, 0x000000000033b5d0): DW_OP_reg5 RDI
Ali>     [0x000000000033b5d0, 0x000000000033b62b): DW_OP_reg15 R15
Ali>              [0x000000000033b631, 0x000000000033b65e): DW_OP_reg15 R15)
Ali>              DW_AT_name       ("ptr")
Ali>              DW_AT_decl_file  ("/path/to/tcmalloc.cc")
Ali>              DW_AT_decl_line  (1385)
Ali>              DW_AT_type       (0x0003d6d6 "*")

Ali> But then:
Ali> partial_die_info::read (  ...
Ali>  switch (attr.name<https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fattr.name%2F&data=02%7C01%7CNitika.Achra%40amd.com%7Cdc148d3943894e037b6a08d7a78d7268%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637162098268321041&sdata=uO7qWPYfgVGf6wlo2QbqaRQTTOof945hTOrJFCGhoJM%3D&reserved=0>) case DW_AT_location:
Ali>      ...
Ali>      else if (attr_form_is_section_offset (&attr)) {
Ali>        dwarf2_complex_location_expr_complaint ();  // ***

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

As per the DWARFv5 standard DW_AT_location is allowed to have DW_FORM_loclistx. So this complaint should be obsolete now if only DW_FORM_loclistx is being used. But whether the section offsets like DW_FORM_sec_offset, DW_FORM_data4 and DW_FORM_data8 can still be emitted under DWARFv5 or not? If yes then the complaint should get fired for them but not for DW_FORM_loclistx. Am I right?

Ali>Thank you,
Ali>Ali

Regards,
Nitika

On Fri, Jan 31, 2020 at 12:55 AM Achra, Nitika <Nitika.Achra@amd.com<mailto: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