[PR,GDB/32398] Fix name resolution for imported D modules

  This fixes the name lookup regression since GDB 10 for the D language
where if an import is nested inside a function declaration, the lookup
will fail due to scope information is missing.
---
 gdb/dwarf2/read.c                   |  6 ++
 gdb/testsuite/gdb.dlang/imports.c   | 34 +++++++++++
 gdb/testsuite/gdb.dlang/imports.exp | 88 +++++++++++++++++++++++++++++
 3 files changed, 128 insertions(+)
 create mode 100644 gdb/testsuite/gdb.dlang/imports.c
 create mode 100644 gdb/testsuite/gdb.dlang/imports.exp
  

Hi,

On 12/10/24 1:40 PM, liushuyu wrote:
> This fixes the name lookup regression since GDB 10 for the D language
> where if an import is nested inside a function declaration, the lookup
> will fail due to scope information is missing.

Thank you for the patch. I am sorry that no one has yet responded to
this -- likely because few of us are proficient in D. Nonetheless,
in an effort to get this patch moving, I'll offer my proverbial $0.02.

BTW, this patch is pretty non-trivial (in its entirety). Do you have
an assignment? I don't see your email mentioned in gdb/MAINTAINERS.

See

https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment

> ---
>   gdb/dwarf2/read.c                   |  6 ++
>   gdb/testsuite/gdb.dlang/imports.c   | 34 +++++++++++
>   gdb/testsuite/gdb.dlang/imports.exp | 88 +++++++++++++++++++++++++++++
>   3 files changed, 128 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.dlang/imports.c
>   create mode 100644 gdb/testsuite/gdb.dlang/imports.exp
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 1ae56d3fb5..48bb09b2b0 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -20377,6 +20377,12 @@ determine_prefix (struct die_info *die, struct dwarf2_cu *cu)
>   	    else if (die->tag == DW_TAG_entry_point)
>   	      return determine_prefix (parent, cu);
>   	  }
> +	/* Imports in D langauge might be nested inside functions or delegates.
> +	   We will bubble up through the tree to find the module name. */

typo: "D language"

> +	if (cu->lang () == language_d)
> +	  {
> +	    return determine_prefix (parent, cu);
> +	  }

There's no need/desire to encapsulate this single statement in
braces.

>   	return "";
>         case DW_TAG_enumeration_type:
>   	parent_type = read_type_die (parent, cu);
> diff --git a/gdb/testsuite/gdb.dlang/imports.c b/gdb/testsuite/gdb.dlang/imports.c
> new file mode 100644
> index 0000000000..8a91f30d0c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dlang/imports.c
> @@ -0,0 +1,34 @@
> +/* Copyright 2017-2024 Free Software Foundation, Inc.

Why 2017-2024? This should likely just be 2025 now.

> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* DWARF will describe these contents as being inside a D module.  */
> +float _D6import6b_Globf = 55.22;

I've read through the associated bug (d/32398), and I can confirm that
the listed reproducer is currently broken, and that this patch fixes it.
Why not use the simple reproducer you reported in the d/32398? I don't
see why using the DWARF assembler is necessary for this particular bug.

> +
> +int
> +_Dmain (void)
> +{
> +  asm ("_Dmain_label: .globl _Dmain_label");
> +  _D6import6b_Globf = 0.125;
> +  asm ("_Dmain_break_label: .globl _Dmain_break_label");
> +  return 0;
> +}
> +
> +asm ("_Dmain_end: .globl _Dmain_end");
> +
> +int
> +main (void)
> +{
> +  return _Dmain ();
> +}
> diff --git a/gdb/testsuite/gdb.dlang/imports.exp b/gdb/testsuite/gdb.dlang/imports.exp
> new file mode 100644
> index 0000000000..936ee94bba
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dlang/imports.exp
> @@ -0,0 +1,88 @@
> +# Copyright (C) 2017-2024 Free Software Foundation, Inc.

Ditto the copyright date.

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test symbol lookup when there are multiple circular imports.
> +
> +load_lib "d-support.exp"
> +load_lib "dwarf.exp"
> +
> +require allow_d_tests dwarf2_support
> +
> +standard_testfile imports.c imports-dw.S
> +
> +lassign [function_range _Dmain ${srcdir}/${subdir}/${srcfile}] \
> +    dmain_start dmain_length
> +
> +
> +# Make some DWARF for the test.
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global dmain_start dmain_length
> +
> +    cu { label cu_start } {
> +	compile_unit {
> +	    {language @DW_LANG_D}
> +	} {
> +	    declare_labels main_module_label imported_module_label float_label
> +
> +            float_label: base_type {
> +		{name float}
> +		{encoding @DW_ATE_float}
> +		{byte_size 4 DW_FORM_sdata}
> +	    }
> +
> +            imported_module_label: module {
> +                {name import}
> +            } {
> +                tag_variable {
> +                    {name b_Globf}
> +                    {MIPS_linkage_name _D6import6b_Globf}
> +                    {external 1 flag}
> +                    {type :$float_label}
> +                }
> +            }
> +
> +	    main_module_label: module {
> +		{name main}
> +	    } {
> +		subprogram {
> +		    {MACRO_AT_func { "_Dmain" }}
> +		    {external 1 flag_present}
> +		} {
> +                    imported_module {
> +                        { import :$imported_module_label }
> +                    }
> +                }
> +	    }
> +	}
> +    }
> +
> +    aranges {} cu_start {
> +	arange {} $dmain_start $dmain_length
> +    }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +          [list $srcfile $asm_file] {nodebug}] } {
> +    return -1
> +}
> +
> +gdb_test_no_output "set language d"
> +
> +if {![runto "_Dmain_break_label"]} {
> +    return -1
> +}
> +
> +gdb_test "print b_Glob" "= 0.125"

Please double-check the output of "git diff --check". There are numerous
whitespace issues noticed by that.

Finally, you should add the "Bug:" trailer for this. [See
gdb/MAINTAINERS for more on the git trailers used by the
project.]

Thanks again for the patch!

Keith
  

Hi Keit,

> Hi,
>
> On 12/10/24 1:40 PM, liushuyu wrote:
>> This fixes the name lookup regression since GDB 10 for the D language
>> where if an import is nested inside a function declaration, the lookup
>> will fail due to scope information is missing.
>
> Thank you for the patch. I am sorry that no one has yet responded to
> this -- likely because few of us are proficient in D. Nonetheless,
> in an effort to get this patch moving, I'll offer my proverbial $0.02.
>
> BTW, this patch is pretty non-trivial (in its entirety). Do you have
> an assignment? I don't see your email mentioned in gdb/MAINTAINERS.
>
> See
>
> https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment 
>
No, I don't have an assignment for the GDB project.
>
>> ---
>>   gdb/dwarf2/read.c                   |  6 ++
>>   gdb/testsuite/gdb.dlang/imports.c   | 34 +++++++++++
>>   gdb/testsuite/gdb.dlang/imports.exp | 88 +++++++++++++++++++++++++++++
>>   3 files changed, 128 insertions(+)
>>   create mode 100644 gdb/testsuite/gdb.dlang/imports.c
>>   create mode 100644 gdb/testsuite/gdb.dlang/imports.exp
>>
>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
>> index 1ae56d3fb5..48bb09b2b0 100644
>> --- a/gdb/dwarf2/read.c
>> +++ b/gdb/dwarf2/read.c
>> @@ -20377,6 +20377,12 @@ determine_prefix (struct die_info *die, 
>> struct dwarf2_cu *cu)
>>           else if (die->tag == DW_TAG_entry_point)
>>             return determine_prefix (parent, cu);
>>         }
>> +    /* Imports in D langauge might be nested inside functions or 
>> delegates.
>> +       We will bubble up through the tree to find the module name. */
>
> typo: "D language"
>
>> +    if (cu->lang () == language_d)
>> +      {
>> +        return determine_prefix (parent, cu);
>> +      }
>
> There's no need/desire to encapsulate this single statement in
> braces.
>
>>       return "";
>>         case DW_TAG_enumeration_type:
>>       parent_type = read_type_die (parent, cu);
>> diff --git a/gdb/testsuite/gdb.dlang/imports.c 
>> b/gdb/testsuite/gdb.dlang/imports.c
>> new file mode 100644
>> index 0000000000..8a91f30d0c
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.dlang/imports.c
>> @@ -0,0 +1,34 @@
>> +/* Copyright 2017-2024 Free Software Foundation, Inc.
>
> Why 2017-2024? This should likely just be 2025 now.
>
I sent the patch in 2024, so the copyright header reads 2024.

I can address this issue in the newer revision of the patch.

>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see 
>> <http://www.gnu.org/licenses/>.  */
>> +
>> +/* DWARF will describe these contents as being inside a D module.  */
>> +float _D6import6b_Globf = 55.22;
>
> I've read through the associated bug (d/32398), and I can confirm that
> the listed reproducer is currently broken, and that this patch fixes it.
> Why not use the simple reproducer you reported in the d/32398? I don't
> see why using the DWARF assembler is necessary for this particular bug.
>
 From what I could see, all the D tests are done using DWARF assembler, 
as GDB test framework currently lacks the machinery to build multi-file 
D projects (which is needed for this test case).

So the next best thing is to use a stub C file and DWARF assembler 
scripts to put together a fake D binary.

>> +
>> +int
>> +_Dmain (void)
>> +{
>> +  asm ("_Dmain_label: .globl _Dmain_label");
>> +  _D6import6b_Globf = 0.125;
>> +  asm ("_Dmain_break_label: .globl _Dmain_break_label");
>> +  return 0;
>> +}
>> +
>> +asm ("_Dmain_end: .globl _Dmain_end");
>> +
>> +int
>> +main (void)
>> +{
>> +  return _Dmain ();
>> +}
>> diff --git a/gdb/testsuite/gdb.dlang/imports.exp 
>> b/gdb/testsuite/gdb.dlang/imports.exp
>> new file mode 100644
>> index 0000000000..936ee94bba
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.dlang/imports.exp
>> @@ -0,0 +1,88 @@
>> +# Copyright (C) 2017-2024 Free Software Foundation, Inc.
>
> Ditto the copyright date.
>
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# Test symbol lookup when there are multiple circular imports.
>> +
>> +load_lib "d-support.exp"
>> +load_lib "dwarf.exp"
>> +
>> +require allow_d_tests dwarf2_support
>> +
>> +standard_testfile imports.c imports-dw.S
>> +
>> +lassign [function_range _Dmain ${srcdir}/${subdir}/${srcfile}] \
>> +    dmain_start dmain_length
>> +
>> +
>> +# Make some DWARF for the test.
>> +set asm_file [standard_output_file $srcfile2]
>> +Dwarf::assemble $asm_file {
>> +    global dmain_start dmain_length
>> +
>> +    cu { label cu_start } {
>> +    compile_unit {
>> +        {language @DW_LANG_D}
>> +    } {
>> +        declare_labels main_module_label imported_module_label 
>> float_label
>> +
>> +            float_label: base_type {
>> +        {name float}
>> +        {encoding @DW_ATE_float}
>> +        {byte_size 4 DW_FORM_sdata}
>> +        }
>> +
>> +            imported_module_label: module {
>> +                {name import}
>> +            } {
>> +                tag_variable {
>> +                    {name b_Globf}
>> +                    {MIPS_linkage_name _D6import6b_Globf}
>> +                    {external 1 flag}
>> +                    {type :$float_label}
>> +                }
>> +            }
>> +
>> +        main_module_label: module {
>> +        {name main}
>> +        } {
>> +        subprogram {
>> +            {MACRO_AT_func { "_Dmain" }}
>> +            {external 1 flag_present}
>> +        } {
>> +                    imported_module {
>> +                        { import :$imported_module_label }
>> +                    }
>> +                }
>> +        }
>> +    }
>> +    }
>> +
>> +    aranges {} cu_start {
>> +    arange {} $dmain_start $dmain_length
>> +    }
>> +}
>> +
>> +if { [prepare_for_testing "failed to prepare" ${testfile} \
>> +          [list $srcfile $asm_file] {nodebug}] } {
>> +    return -1
>> +}
>> +
>> +gdb_test_no_output "set language d"
>> +
>> +if {![runto "_Dmain_break_label"]} {
>> +    return -1
>> +}
>> +
>> +gdb_test "print b_Glob" "= 0.125"
>
> Please double-check the output of "git diff --check". There are numerous
> whitespace issues noticed by that.
>
> Finally, you should add the "Bug:" trailer for this. [See
> gdb/MAINTAINERS for more on the git trailers used by the
> project.]
>
Will address these issues.
> Thanks again for the patch!
>
> Keith
>
Thanks,

Zixing