[PATCH] Support for DW_AT_loclists_base and DW_FORM_loclistx.

[Tom Tromey]

>>>>> "Nitika" == Achra, Nitika <Nitika.Achra@amd.com> writes:

Nitika> *Support for DW_AT_loclists_base and DW_FORM_loclistx.

Thanks for the patch.

My comments below are primarily nits.

Nitika> Tested by running the testsuite before and after the patch and there
Nitika> is no increase in the number of test cases that fails. Tested with both
Nitika> -gdwarf-4 and -gdwarf-5 flags. Also tested -gslit-dwarf along with
Nitika> -gdwarf-4 as well as -gdwarf5 flags.

I assume it fixed some tests with -gdwarf-5?  Or else we'll need a new
test case.

Nitika> +/* Size of .debug_loclist section header for 32-bit DWARF format. */
Nitika> +#define LOCLIST_HEADER_SIZE32 12;
Nitika> +
Nitika> +/* Size of .debug_loclist section header for 64-bit DWARF format. */
Nitika> +#define LOCLIST_HEADER_SIZE64 20;

The ";" at the end of these defines is weird.

Nitika> +  /* Header data from the location list section. */
Nitika> +  struct loclist_header* loclist_header;

gdb style puts the "*" on the other side of the " " like

    struct loclist_header *loclist_header;

Nitika> +static struct dwarf2_section_info* cu_debug_loc_section (struct dwarf2_cu* cu);

Here too -- there are actually several instances in the patch.

Nitika> +void
Nitika> +read_loclist_header (struct dwarf2_cu* cu, struct dwarf2_section_info* section)
Nitika> +{

New functions should have an introductory comment explaining their purpose.

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 declare them if the uses come after the
definition, so probably some of those declarations can be removed as
well.

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.

Nitika> +ULONGEST
Nitika> +lookup_loclist_base (struct dwarf2_cu* cu)
Nitika> +{
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;

Is there some way to avoid hard-coding sizes here?

Nitika> +/* Given a DW_FORM_loclistx value loclist_index, fetch the offset from the array
Nitika> +   of offsets in the .debug_loclists section. */
Nitika> +CORE_ADDR
Nitika> +read_loclist_index (struct dwarf2_cu* cu, ULONGEST loclist_index)
Nitika> +{
...
Nitika> +  const gdb_byte* info_ptr;

This can be declared later, when it's first initialized.

Nitika> +  if (section->buffer == NULL)
Nitika> +    error(_("DW_FORM_loclistx used without .debug_loclist section [in module %s]"),
Nitika> +	objfile_name (objfile));

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.
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 will be aborted.  (It can even cause worse problems,
there's a bug in bugzilla about it ending a remote session.)

Maybe complaint() and then a fallback would be preferable.
Or, test the error() to make sure it's ok.

Nitika> +  delete cu->loclist_header;

This is created, only to be deleted in the same function.
I think it would be better to just stack-allocate this.

Or, should this be cached in the CU -- that is, read once and then reused?
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, &bytes_read);

Space before the first "(".

Tom