Re: [PATCH] Debug support for global alias variable

[Bruno Larsen <blarsen@redhat.com>]

Hello Kavitha!


Thanks for looking at this! I have taken a look and have some comments. I couldn't really test if it fixed the issue since it seems that the patch is not merged yet and I don't really know how to work with LLVM's patch review format, but I have tested for regressions on x86_64 and I see no regressions.

About the patch idea itself, I agree that imported_declaration looks like a good fit for an aliased variable, so I like the general direction of this patch.

Also, I can't approve any patches, but I hope this review helps get the conversation going.

On 3/4/22 10:44, Natarajan, Kavitha via Gdb-patches wrote:
> [AMD Official Use Only]
> 
> Hi,
> 
> For the below testcase, when compiled with clang compiler, debugger is not able to print alias variable type or value.
> $ cat test.c
> int oldname = 1;
> extern int newname attribute((alias("oldname")));
> int main ()
> { return 0; }
> $ clang -g -O0 test.c
> $ gdb a.out
> (gdb) ptype oldname
> type = int
> (gdb) ptype newname
> type = <data variable, no debug info>
> (gdb) p oldname
> $1 = 1
> (gdb) p newname
> 'newname' has unknown type; cast it to its declared type
> This is because clang is not emitting dwarf information for alias variable. I have updated clang patch (originally from https://reviews.llvm.org/D103131) to emit debug info for alias variable as imported entity (DW_TAG_imported_declaration):
> 
> https://reviews.llvm.org/D120989
> 
> However, gdb cannot handle the imported declaration for alias variables. GCC emits debug info for alias variables as DW_TAG_variable which gdb can handle. The discussions in the llvm bug report bug-50052<https://bugs.llvm.org/show_bug.cgi?id=50052> and above review links talk about why it is appropriate to emit alias variable as DW_TAG_imported_declaration rather than DW_TAG_variable. Refer section "3.2.3 Imported (or Renamed) Declaration Entries" in DWARF 5 specification. It was also previously discussed in gdb forum in the below thread:
> 
> https://sourceware.org/pipermail/gdb/2021-June/049509.html
> 
> The attached gdb patch can handle DW_TAG_imported_declaration as alias variables. This change fixes the failure in testcase:
>             gdb.base/symbol-alias.exp
> This testcase is also updated to test nested (recursive) alias, static aliasee and deferred aliasee.

> 
> Please review the below gdb patch (also attached).


> 
> ---
> gdb/dwarf2/read.c                       | 113 ++++++++++++++++--------
> gdb/testsuite/gdb.base/symbol-alias.exp |  23 ++++-
> gdb/testsuite/gdb.base/symbol-alias2.c  |  10 +++
> 3 files changed, 107 insertions(+), 39 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 10550336063..0f709d9ccc1 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -1132,6 +1132,10 @@ static void add_partial_subprogram (struct partial_die_info *pdi,
>                                      CORE_ADDR *lowpc, CORE_ADDR *highpc,
>                                      int need_pc, struct dwarf2_cu *cu);
> 
> +static void var_decode_location (struct attribute *attr,
> +                                 struct symbol *sym,
> +                                 struct dwarf2_cu *cu);
> +

Could you have these have tabs instead of 8 spaces? This goes for all indentations that have at least 8 spaces, by the way.

> static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
> 
> static struct partial_die_info *load_partial_dies
> @@ -1332,7 +1336,7 @@ static struct using_direct **using_directives (struct dwarf2_cu *cu);
> 
> static void read_import_statement (struct die_info *die, struct dwarf2_cu *);
> 
> -static int read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu);
> +static int read_alias (struct die_info *die, struct dwarf2_cu *cu);
> 
> static struct type *read_module_type (struct die_info *die,
>                                        struct dwarf2_cu *cu);
> @@ -9742,9 +9746,11 @@ process_die (struct die_info *die, struct dwarf2_cu *cu)
>         break;
>       case DW_TAG_imported_declaration:
>         cu->processing_has_namespace_info = true;

Since we are using imported declarations for more than just namespaces, this line is not necessarily true always. I think read_alias is going to have to do this, but a comment explaining why this case is different to the ones around it is welcome.

> -      if (read_namespace_alias (die, cu))
> +      if (((cu->per_cu->lang == language_c)
> +            || (cu->per_cu->lang == language_cplus))
> +                            && read_alias (die, cu))

Could you align the && with the second open parenthesis of the if line? This would make the whole expression easier to read.

>          break;
> -      /* The declaration is not a global namespace alias.  */
> +      /* The declaration is not a global namespace or variable alias. */

We require 2 spaces after the period, even at the end of comments.

>         /* Fall through.  */
>       case DW_TAG_imported_module:
>         cu->processing_has_namespace_info = true;
> @@ -10212,18 +10218,20 @@ dwarf2_physname (const char *name, struct die_info *die, struct dwarf2_cu *cu)
>     return retval;
> }
> 
> -/* Inspect DIE in CU for a namespace alias.  If one exists, record
> -   a new symbol for it.
> +#define MAX_NESTED_IMPORTED_DECLARATIONS 100
> +
> +/* Inspect DIE in CU for a namespace alias or a variable
> +   with alias attribute. If one exists, record a new symbol
> +   for it.
> 
> -   Returns 1 if a namespace alias was recorded, 0 otherwise.  */
> +   Returns 1 if an alias was recorded, 0 otherwise. */

Same here.

> 
> static int
> -read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu)
> +read_alias (struct die_info *die, struct dwarf2_cu *cu)
> {
>     struct attribute *attr;
> 
> -  /* If the die does not have a name, this is not a namespace
> -     alias.  */
> +  /* The die must have a name for namespace or variable alias */

And here. Also, please be sure to add periods at the end of comments.

>     attr = dwarf2_attr (die, DW_AT_name, cu);
>     if (attr != NULL)
>       {
> @@ -10232,42 +10240,71 @@ read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu)
>         struct dwarf2_cu *imported_cu = cu;
> 
>         /* If the compiler has nested DW_AT_imported_declaration DIEs,
> -        keep inspecting DIEs until we hit the underlying import.  */
> -#define MAX_NESTED_IMPORTED_DECLARATIONS 100
> +         keep inspecting DIEs until we hit the underlying import. */

And here.

>         for (num = 0; num  < MAX_NESTED_IMPORTED_DECLARATIONS; ++num)
> -       {
> -         attr = dwarf2_attr (d, DW_AT_import, cu);
> -         if (attr == NULL)
> -           break;
> +        {
> +          attr = dwarf2_attr (d, DW_AT_import, cu);
> +          if (attr == NULL)
> +            return 0;
> 
> -         d = follow_die_ref (d, attr, &imported_cu);
> -         if (d->tag != DW_TAG_imported_declaration)
> -           break;
> -       }
> +          d = follow_die_ref (d, attr, &imported_cu);
> +          if (d->tag != DW_TAG_imported_declaration)
> +            break;
> +        }
> 
>         if (num == MAX_NESTED_IMPORTED_DECLARATIONS)
> -       {
> -         complaint (_("DIE at %s has too many recursively imported "
> -                      "declarations"), sect_offset_str (d->sect_off));
> -         return 0;
> -       }
> +        {
> +          complaint (_("DIE at %s has too many recursively imported "
> +                       "declarations"), sect_offset_str (d->sect_off));
> +          return 0;
> +        }
> 
>         if (attr != NULL)
> -       {
> -         struct type *type;
> -         sect_offset sect_off = attr->get_ref_die_offset ();
> -
> -         type = get_die_type_at_offset (sect_off, cu->per_cu, cu->per_objfile);
> -         if (type != NULL && type->code () == TYPE_CODE_NAMESPACE)
> -           {
> -             /* This declaration is a global namespace alias.  Add
> -                a symbol for it whose type is the aliased namespace.  */
> -             new_symbol (die, type, cu);
> -             return 1;
> -           }
> -       }
> +        {
> +          struct type *type;
> +          if (d->tag == DW_TAG_variable)
> +            {
> +             /* This declaration is a global variable alias. Add
> +                a symbol for it whose type is same as aliased variable. */
> +              type = die_type (d, imported_cu);
> +              struct symbol *sym = new_symbol(die, type, cu);
> +              attr = dwarf2_attr (d, DW_AT_location, imported_cu);
> +              if (attr != nullptr)
> +                {
> +                  var_decode_location (attr, sym, cu);
> +                  return 1;
> +                }
> +              else
> +                {
> +                  /* We do not know the address of this symbol.
> +                     If it is an external symbol and we have type information
> +                     for it, enter the symbol as a LOC_UNRESOLVED symbol.
> +                     The address of the variable will then be determined from
> +                     the minimal symbol table whenever the variable is
> +                     referenced.  */
> +                  attr = dwarf2_attr (d, DW_AT_external, imported_cu);
> +                  if (attr != nullptr && attr->as_boolean ()
> +                      && dwarf2_attr (d, DW_AT_type, imported_cu) != NULL)
> +                    {
> +                      sym->set_aclass_index (LOC_UNRESOLVED);
> +                      return 1;

If you look at the original code, you see that if a new symbol was allocated, the return is always 1. I think having a possibility that the symbol can be allocated and we still return 0 could be a problem, but I am not sure. Regardless, by this point (and the if block above) we are pretty sure that the DIE is indeed a namespace variable or a variable alias, so returning 0 if we can't find the location seems wrong, but I am not a DWARF reader expert, so take my ramblings with a grain of salt.

<side note> why is this function returning 1 or 0 as boolean, instead of the function returning bool?</side note>

> +                    }
> +                }
> +            }
> +          else
> +            {
> +              sect_offset sect_off = attr->get_ref_die_offset ();
> +              type = get_die_type_at_offset (sect_off, cu->per_cu, cu->per_objfile);
> +              if (type != NULL && type->code () == TYPE_CODE_NAMESPACE)
> +                {
> +                  /* This declaration is a global namespace alias. Add
> +                     a symbol for it whose type is the aliased namespace. */
> +                  new_symbol (die, type, cu);
> +                  return 1;
> +                }
> +            }
> +        }

Is this "else" path being exercised anywhere? For this simple code:

namespace n{
     int i;
     int j;
     void f(){ }
}

using n::i;
using n::f;

int var;
extern int newvar __attribute__((alias("var")));

int main(){
     i = 0;
     return 0;
}

g++ makes the following dwarf information:

  <1><2e>: Abbrev Number: 5 (DW_TAG_namespace)
     <2f>   DW_AT_name        : n
     <31>   DW_AT_decl_file   : 1
     <32>   DW_AT_decl_line   : 1
     <33>   DW_AT_decl_column : 11
     <34>   DW_AT_sibling     : <0x5b>
  <2><38>: Abbrev Number: 1 (DW_TAG_variable)
     <39>   DW_AT_name        : i
     <3b>   DW_AT_decl_file   : 1
     <3b>   DW_AT_decl_line   : 2
     <3c>   DW_AT_decl_column : 9
     <3c>   DW_AT_linkage_name: (indirect string, offset: 0x67): _ZN1n1iE
     <40>   DW_AT_type        : <0x5b>
     <44>   DW_AT_external    : 1
     <44>   DW_AT_declaration : 1
  <2><44>: Abbrev Number: 1 (DW_TAG_variable)
     <45>   DW_AT_name        : j
     <47>   DW_AT_decl_file   : 1
     <47>   DW_AT_decl_line   : 3
     <48>   DW_AT_decl_column : 9
     <48>   DW_AT_linkage_name: (indirect string, offset: 0x70): _ZN1n1jE
     <4c>   DW_AT_type        : <0x5b>
     <50>   DW_AT_external    : 1
     <50>   DW_AT_declaration : 1
  <2><50>: Abbrev Number: 6 (DW_TAG_subprogram)
     <51>   DW_AT_external    : 1
     <51>   DW_AT_name        : f
     <53>   DW_AT_decl_file   : 1
     <54>   DW_AT_decl_line   : 4
     <55>   DW_AT_decl_column : 10
     <56>   DW_AT_linkage_name: (indirect string, offset: 0x5d): _ZN1n1fEv
     <5a>   DW_AT_declaration : 1
  <2><5a>: Abbrev Number: 0
  <1><5b>: Abbrev Number: 7 (DW_TAG_base_type)
     <5c>   DW_AT_byte_size   : 4
     <5d>   DW_AT_encoding    : 5        (signed)
     <5e>   DW_AT_name        : int
  <1><62>: Abbrev Number: 2 (DW_TAG_variable)
     <63>   DW_AT_specification: <0x38>
     <67>   DW_AT_location    : 9 byte block: 3 20 40 40 0 0 0 0 0       (DW_OP_addr: 404020)
  <1><71>: Abbrev Number: 2 (DW_TAG_variable)
     <72>   DW_AT_specification: <0x44>
     <76>   DW_AT_location    : 9 byte block: 3 24 40 40 0 0 0 0 0       (DW_OP_addr: 404024)
  <1><80>: Abbrev Number: 3 (DW_TAG_imported_declaration)
     <81>   DW_AT_decl_file   : 1
     <81>   DW_AT_decl_line   : 7
     <82>   DW_AT_decl_column : 10
     <82>   DW_AT_import      : <0x62>   [Abbrev Number: 2 (DW_TAG_variable)]
  <1><86>: Abbrev Number: 3 (DW_TAG_imported_declaration)
     <87>   DW_AT_decl_file   : 1
     <87>   DW_AT_decl_line   : 8
     <88>   DW_AT_decl_column : 10
     <88>   DW_AT_import      : <0x50>   [Abbrev Number: 6 (DW_TAG_subprogram)]
  <1><8c>: Abbrev Number: 8 (DW_TAG_variable)
     <8d>   DW_AT_name        : var
     <91>   DW_AT_decl_file   : 1
     <92>   DW_AT_decl_line   : 10
     <93>   DW_AT_decl_column : 5
     <94>   DW_AT_type        : <0x5b>
     <98>   DW_AT_external    : 1
     <98>   DW_AT_location    : 9 byte block: 3 28 40 40 0 0 0 0 0       (DW_OP_addr: 404028)
  <1><a2>: Abbrev Number: 9 (DW_TAG_variable)
     <a3>   DW_AT_name        : (indirect string, offset: 0x51): newvar
     <a7>   DW_AT_decl_file   : 1
     <a8>   DW_AT_decl_line   : 11
     <a9>   DW_AT_decl_column : 12
     <aa>   DW_AT_type        : <0x5b>
     <ae>   DW_AT_external    : 1
  <1><ae>: Abbrev Number: 10 (DW_TAG_subprogram)
     <af>   DW_AT_external    : 1
     <af>   DW_AT_name        : (indirect string, offset: 0x58): main
     <b3>   DW_AT_decl_file   : 1
     <b4>   DW_AT_decl_line   : 13
     <b5>   DW_AT_decl_column : 5
     <b6>   DW_AT_type        : <0x5b>
     <ba>   DW_AT_low_pc      : 0x40110d
     <c2>   DW_AT_high_pc     : 0x15
     <ca>   DW_AT_frame_base  : 1 byte block: 9c         (DW_OP_call_frame_cfa)
     <cc>   DW_AT_call_all_calls: 1
  <1><cc>: Abbrev Number: 11 (DW_TAG_subprogram)
     <cd>   DW_AT_specification: <0x50>
     <d1>   DW_AT_low_pc      : 0x401106
     <d9>   DW_AT_high_pc     : 0x7
     <e1>   DW_AT_frame_base  : 1 byte block: 9c         (DW_OP_call_frame_cfa)
     <e3>   DW_AT_call_all_calls: 1
  <1><e3>: Abbrev Number: 0


as you can see, the DW_TAG_imported_declaration's DW_AT_import points at the DW_TAG_variable at the global scope, and even though the details of the variable are inside the namespace, the declaration is global.  And imported functions does not use DW_TAG_imported_declaration, so I can't really see when we would use that else block. I think this is a long winded way of asking "can this be removed?".

>       }
> -
>     return 0;
> }
> 
> diff --git a/gdb/testsuite/gdb.base/symbol-alias.exp b/gdb/testsuite/gdb.base/symbol-alias.exp
> index 2b53cc31053..1259c1daf1d 100644
> --- a/gdb/testsuite/gdb.base/symbol-alias.exp
> +++ b/gdb/testsuite/gdb.base/symbol-alias.exp
> @@ -15,6 +15,7 @@
> # along with this program.  If not, see http://www.gnu.org/licenses/.
> 
> standard_testfile symbol-alias.c symbol-alias2.c
> +set using_clang [test_compiler_info clang*]

Since this is a new feature of clang and some tests run on quite old systems, could you try and detect the clang version, so you can have xfails for all aliases if the clang version doesn't emit this information?

> 
> if { [prepare_for_testing "failed to prepare" ${testfile} [list $srcfile $srcfile2]] } {
>       return -1
> @@ -31,6 +32,26 @@ foreach f {"func" "func_alias"} {
> }
> 
> # Variables.
> -foreach v {"g_var_s" "g_var_s_alias"} {
> +foreach v {"g_var_s" "g_var_s_alias" "g_var_s_alias2"} {
>       gdb_test "p $v" "= {field1 = 1, field2 = 2}"
> }
> +
> +# Static Variable.
> +foreach v {"g_var" "g_var_alias"} {
> +    gdb_test "p $v" " = 1"
> +}
> +
> +# Deferred Variable.
> +gdb_test "p g_def_var" " = 2"
> +gdb_test_multiple "p g_def_var_alias" "p g_def_var_alias" {
> +    -re  " = 2.*$gdb_prompt $" {
> +        pass "print alias of deferred variable"
> +    }
> +    -re  ".*has unknown type; cast it to its declared type.*$gdb_prompt $" {
> +        if { $using_clang } {
> +          xfail "print alias of deferred variable"
> +        } else {
> +          fail "print alias of deferred variable"
> +        }
> +    }
> +}
> diff --git a/gdb/testsuite/gdb.base/symbol-alias2.c b/gdb/testsuite/gdb.base/symbol-alias2.c
> index 34f4e121e25..79b3195caf5 100644
> --- a/gdb/testsuite/gdb.base/symbol-alias2.c
> +++ b/gdb/testsuite/gdb.base/symbol-alias2.c
> @@ -23,6 +23,8 @@ struct S
> 
> struct S g_var_s = { 1, 2 };
> 
> +static int g_var = 1;
> +
> #ifdef __cplusplus
> /* So that the alias attribute below work without having to figure out
>      this function's mangled name.  */
> @@ -38,3 +40,11 @@ func (void)
> struct S *func_alias (void) __attribute__ ((alias ("func")));
> 
> extern struct S g_var_s_alias __attribute__ ((alias ("g_var_s")));
> +
> +extern struct S g_var_s_alias2 __attribute__ ((alias ("g_var_s_alias")));
> +
> +extern int g_var_alias __attribute__ ((alias ("g_var")));
> +
> +extern int g_def_var_alias __attribute__ ((alias ("g_def_var")));
> +
> +int g_def_var = 2;
> --
> 


-- 
Cheers!
Bruno Larsen