[v2] gdb: Fix issue with Clang CLI macros

  Clang up to the current version adds macros that were defined in the
command line or by "other means", according to the Dwarf specification,
after the last DW_MACRO_end_file, instead of before the first
DW_MACRO_start_file, as the specification dictates.  When GDB reads the
macros after the last file is closed, the macros never end up "in scope"
and so we can't print them.  This has been submitted as a bug to Clang
developers, and PR macros/29034 was opened for GDB to keep track of
this.

Seeing as there is no expected date for it to be fixed, a workaround was
added for all current versions of Clang.  The workaround detects when
the main file would be closed and if the producer is clang, and turns
that operation into a noop, so we keep a reference to the current_file
as those macros are read.

A test case was added to confirm the functionality.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29034
---
 gdb/dwarf2/cu.h                               |   1 +
 gdb/dwarf2/macro.c                            |  18 +++-
 gdb/dwarf2/macro.h                            |   2 +-
 gdb/dwarf2/read.c                             |  15 ++-
 gdb/dwarf2/read.h                             |   2 +
 gdb/producer.c                                |  26 +++++
 gdb/producer.h                                |   4 +
 gdb/testsuite/gdb.dwarf2/clang_cli_macros.exp | 101 ++++++++++++++++++
 8 files changed, 163 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/clang_cli_macros.exp
  

Hi Bruno,

Thanks for fixing this.

On 2022-10-17 06:26, Bruno Larsen via Gdb-patches wrote:
> Clang up to the current version adds macros that were defined in the

Might as well say what is the current version.

> command line or by "other means", according to the Dwarf specification,
> after the last DW_MACRO_end_file, instead of before the first
> DW_MACRO_start_file, as the specification dictates.  When GDB reads the
> macros after the last file is closed, the macros never end up "in scope"
> and so we can't print them.  This has been submitted as a bug to Clang
> developers, and PR macros/29034 was opened for GDB to keep track of
> this.

Can you add the link to the Clang bug?

> 
> Seeing as there is no expected date for it to be fixed, a workaround was
> added for all current versions of Clang.  The workaround detects when

Nit commit message style comment, when talking about the work done in
the current patch, use the present tense:

    Seeing as there is no expected date for it to be fixed, add a workaround
    for all current versions of Clang.

> the main file would be closed and if the producer is clang, and turns
> that operation into a noop, so we keep a reference to the current_file
> as those macros are read.
> 
> A test case was added to confirm the functionality.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29034
> ---
>  gdb/dwarf2/cu.h                               |   1 +
>  gdb/dwarf2/macro.c                            |  18 +++-
>  gdb/dwarf2/macro.h                            |   2 +-
>  gdb/dwarf2/read.c                             |  15 ++-
>  gdb/dwarf2/read.h                             |   2 +
>  gdb/producer.c                                |  26 +++++
>  gdb/producer.h                                |   4 +
>  gdb/testsuite/gdb.dwarf2/clang_cli_macros.exp | 101 ++++++++++++++++++
>  8 files changed, 163 insertions(+), 6 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/clang_cli_macros.exp
> 
> diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
> index 23cb3d21b2e..51638e71b19 100644
> --- a/gdb/dwarf2/cu.h
> +++ b/gdb/dwarf2/cu.h
> @@ -264,6 +264,7 @@ struct dwarf2_cu
>    bool producer_is_icc : 1;
>    bool producer_is_icc_lt_14 : 1;
>    bool producer_is_codewarrior : 1;
> +  bool producer_is_clang : 1;
>  
>    /* When true, the file that we're processing is known to have
>       debugging info for C++ namespaces.  GCC 3.3.x did not produce
> diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
> index 38c0fdfec73..c753dc300f2 100644
> --- a/gdb/dwarf2/macro.c
> +++ b/gdb/dwarf2/macro.c
> @@ -445,7 +445,7 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
>  			  struct dwarf2_section_info *str_section,
>  			  struct dwarf2_section_info *str_offsets_section,
>  			  gdb::optional<ULONGEST> str_offsets_base,
> -			  htab_t include_hash)
> +			  htab_t include_hash, struct dwarf2_cu *cu)
>  {
>    struct objfile *objfile = per_objfile->objfile;
>    enum dwarf_macro_record_type macinfo_type;
> @@ -672,6 +672,16 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
>  	  if (! current_file)
>  	    complaint (_("macro debug info has an unmatched "
>  			 "`close_file' directive"));
> +	  else if (current_file->included_by == nullptr
> +		   && producer_is_clang (cu))
> +	    {
> +	      /* Clang, until the current version, misplaces macro definitions,

Precision: macros defined on the command-line, not all macros.

> +		 putting them after the last DW_MACRO_end_file instead of
> +		 before the first DW_MACRO_start_file.  */
> +	      /* FIXME: Since at the time of writing there is no clang version
> +		 with this bug fixed, we check for any clang producer.  This
> +		 should be changed to producer_is_clang_lt_XX when possible. */

The second comment is relevant, but I would remove the FIXME, as it's
not a known deficiency in GDB.  Also, I think you can merge the two
comments into a single one.

> @@ -13343,6 +13352,10 @@ check_producer (struct dwarf2_cu *cu)
>      }
>    else if (startswith (cu->producer, "CodeWarrior S12/L-ISA"))
>      cu->producer_is_codewarrior = true;
> +  else if (producer_is_clang (cu->producer, &major, &minor))
> +    {
> +      cu->producer_is_clang = true;
> +    }

Remove braces.

>    else
>      {
>        /* For other non-GCC compilers, expect their behavior is DWARF version
> @@ -23363,7 +23376,7 @@ dwarf_decode_macros (struct dwarf2_cu *cu, unsigned int offset,
>  
>    dwarf_decode_macros (per_objfile, builder, section, lh,
>  		       offset_size, offset, str_section, str_offsets_section,
> -		       str_offsets_base, section_is_gnu);
> +		       str_offsets_base, section_is_gnu, cu);
>  }
>  
>  /* Return the .debug_loc section to use for CU.
> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
> index 5f01fbc1025..a47e6bf5144 100644
> --- a/gdb/dwarf2/read.h
> +++ b/gdb/dwarf2/read.h
> @@ -761,4 +761,6 @@ extern void dwarf2_get_section_info (struct objfile *,
>  				     asection **, const gdb_byte **,
>  				     bfd_size_type *);
>  
> +extern bool producer_is_clang (struct dwarf2_cu *cu);
> +
>  #endif /* DWARF2READ_H */
> diff --git a/gdb/producer.c b/gdb/producer.c
> index ef1dd93afbc..a9fb7dd8245 100644
> --- a/gdb/producer.c
> +++ b/gdb/producer.c
> @@ -127,6 +127,32 @@ producer_is_llvm (const char *producer)
>  				 || startswith (producer, " F90 Flang ")));
>  }
>  
> +/* see producer.h */

/* See producer.h.  */

> +
> +bool
> +producer_is_clang (const char* producer, int *major, int* minor)

Space before * (twice).

> +{
> +  const char *cs;

Declare where initialized.

> +
> +  if (producer != nullptr && startswith (producer, "clang "))
> +    {
> +      int maj, min;
> +      if (major == nullptr)
> +	major = &maj;
> +      if (minor == nullptr)
> +	minor = &min;
> +
> +      /* The full producer sting will look something like

sting -> string

> +	 "clang version XX.X.X ..."
> +	 So we can safely ignore all characters before the first digit.  */
> +      cs = producer + strlen("clang version ");

Space before strlen.

Above, you check that the string starts with "clang ", but here you
assume it contains "clang version ".  If you crafted a binary with
"clang " as a producer string, it would make an out-of-bounds access.
I'd suggest changing the startswith call to check for "clang version "
too.  Then, if you crafted a binary with "clang version " as the
producer, I think the sscanf below wouldn't make an out-of-bounds
access, it would just fail (as it would try to parse an empty string).

> +
> +      if (sscanf (cs, "%d.%d", major, minor) == 2)
> +	  return true

One indentation level too much.

;
> +    }
> +  return false;
> +}
> +
>  #if defined GDB_SELF_TEST
>  namespace selftests {
>  namespace producer {
> diff --git a/gdb/producer.h b/gdb/producer.h
> index f7c19368bc6..b75cfae6569 100644
> --- a/gdb/producer.h
> +++ b/gdb/producer.h
> @@ -41,4 +41,8 @@ extern bool producer_is_icc (const char *producer, int *major, int *minor);
>     false otherwise.*/
>  extern bool producer_is_llvm (const char *producer);
>  
> +/* Returns true if the given PRODUCER string is clang, false otherwise.
> +   Sets MAJOR and MINOR accordingly, if not NULL.  */
> +extern bool producer_is_clang (const char *producer, int *major, int *minor);
> +
>  #endif
> diff --git a/gdb/testsuite/gdb.dwarf2/clang_cli_macros.exp b/gdb/testsuite/gdb.dwarf2/clang_cli_macros.exp
> new file mode 100644
> index 00000000000..f056d8ad832
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/clang_cli_macros.exp

We use more kebab case than snake case in test file names.

> @@ -0,0 +1,101 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# 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 that GDB is able to print macros defined in the command line invocaion
> +# of clang. It has to be explicitly tested because of:

Two spaces after period.

> +# https://github.com/llvm/llvm-project/issues/54506
> +
> +load_lib dwarf.exp
> +
> +if {![dwarf2_support]} {
> +    return 0
> +}
> +if {![test_compiler_info gcc-*-*]} {
> +    untested "dwarf assembler needs GCC"
> +}
> +
> +standard_testfile .S
> +
> +set srcfile macro-source-path.c

copy-pasto.  Actually, can you use:

  standard_testfile .c .S

and then use $srcfile for the .c, and $srcfile2 (which you can reassign
to $asm_file for clarity) for the .S?

> +set asm_file [standard_output_file clang_cli_macros.S]

It's preferable not to hard-code source file names, so the test keeps
working if renamed.

> +
> +lassign [function_range main $srcdir/$subdir/$srcfile] \
> +    main_start main_len
> +
> +Dwarf::assemble $asm_file {
> +    global srcfile

I prefer not using the global keyword, but use the $::srcfile form
instead.  It avoids stale "global" definitions, and it shows at the
point of use that the variable is a global one.

> +    declare_labels L cu_macros
> +
> +    cu {} {
> +	DW_TAG_compile_unit {
> +	    {DW_AT_producer "clang version 15.0.0"}
> +	    {DW_AT_language @DW_LANG_C11}
> +	    {DW_AT_name $srcfile}
> +	    {DW_AT_macros $cu_macros DW_FORM_sec_offset}
> +	    {DW_AT_stmt_list $L DW_FORM_sec_offset}
> +	} {
> +	    declare_labels int_type
> +
> +	    int_type: DW_TAG_base_type {
> +	        {DW_AT_byte_size 4 DW_FORM_sdata}
> +	        {DW_AT_encoding  @DW_ATE_signed}
> +	        {DW_AT_name int}
> +	    }
> +	    DW_TAG_subprogram {
> +	        {MACRO_AT_func {main}}
> +	        {type :$int_type}
> +	    }
> +	}
> +    }
> +    lines {version 2} L {
> +	file_name $srcfile 1
> +	program {
> +	    DW_LNE_set_address $::main_start
> +	    line 10
> +	    DW_LNS_copy
> +	    DW_LNE_set_address "$::main_start + $::main_len"
> +	    DW_LNE_end_sequence
> +	}
> +    }
> +
> +    # Define the .debug_macro section.
> +    macro {
> +	cu_macros: unit {
> +	    "debug-line-offset-label" $L
> +        } {
> +	    # Clang has this bug where it puts the macros defined on
> +	    # the command-line after the main file portion (see
> +	    # PR 29034).

I would put that information in the intro comment for the test, where
you link to the llvm bug tracker.

> +	    start_file 0 1
> +		# A macro defined at line 1 of the main file.
> +		define 1 "TWO 2"
> +	    end_file
> +	    define 0 "ONE 1"
> +	}
> +    }
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile [list $srcfile $asm_file] {nodebug}]} {
> +    return
> +}
> +
> +if {![runto_main]} {
> +    return
> +}
> +
> +gdb_test "print TWO" "= 2" "printing simple macro"
> +gdb_test "print ONE" "= 1" "printing defined from CLI"

printing -> print

Thanks,

Simon
  

On 2022-10-17 06:26, Bruno Larsen via Gdb-patches wrote:
> Clang up to the current version adds macros that were defined in the
> command line or by "other means", according to the Dwarf specification,
> after the last DW_MACRO_end_file, instead of before the first
> DW_MACRO_start_file, as the specification dictates.  When GDB reads the
> macros after the last file is closed, the macros never end up "in scope"
> and so we can't print them.  This has been submitted as a bug to Clang
> developers, and PR macros/29034 was opened for GDB to keep track of
> this.
> 
> Seeing as there is no expected date for it to be fixed, a workaround was
> added for all current versions of Clang.  The workaround detects when
> the main file would be closed and if the producer is clang, and turns
> that operation into a noop, so we keep a reference to the current_file
> as those macros are read.
> 
> A test case was added to confirm the functionality.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29034

I forgot to mention, should this setup_kfail be removed?

https://gitlab.com/gnutools/binutils-gdb/-/blob/e57f7fa070e16683c0e1ddd0d1307d243fad990d/gdb/testsuite/gdb.base/macro-source-path.exp#L68

Simon
  

On 17/10/2022 15:22, Simon Marchi wrote:
>
> On 2022-10-17 06:26, Bruno Larsen via Gdb-patches wrote:
>> Clang up to the current version adds macros that were defined in the
>> command line or by "other means", according to the Dwarf specification,
>> after the last DW_MACRO_end_file, instead of before the first
>> DW_MACRO_start_file, as the specification dictates.  When GDB reads the
>> macros after the last file is closed, the macros never end up "in scope"
>> and so we can't print them.  This has been submitted as a bug to Clang
>> developers, and PR macros/29034 was opened for GDB to keep track of
>> this.
>>
>> Seeing as there is no expected date for it to be fixed, a workaround was
>> added for all current versions of Clang.  The workaround detects when
>> the main file would be closed and if the producer is clang, and turns
>> that operation into a noop, so we keep a reference to the current_file
>> as those macros are read.
>>
>> A test case was added to confirm the functionality.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29034
> I forgot to mention, should this setup_kfail be removed?
>
> https://gitlab.com/gnutools/binutils-gdb/-/blob/e57f7fa070e16683c0e1ddd0d1307d243fad990d/gdb/testsuite/gdb.base/macro-source-path.exp#L68
Good catch! yeah, it should be removed, I'll add it in my next version.


Cheers,
Bruno

>
> Simon
>
  

Hi Simon,

thanks for the review. I've fixed everything else you mentioned, but:

On 17/10/2022 14:32, Simon Marchi wrote:
>> +#https://github.com/llvm/llvm-project/issues/54506
>> +
>> +load_lib dwarf.exp
>> +
>> +if {![dwarf2_support]} {
>> +    return 0
>> +}
>> +if {![test_compiler_info gcc-*-*]} {
>> +    untested "dwarf assembler needs GCC"
>> +}
>> +
>> +standard_testfile .S
>> +
>> +set srcfile macro-source-path.c
> copy-pasto.  Actually, can you use:
>
>    standard_testfile .c .S
>
> and then use $srcfile for the .c, and $srcfile2 (which you can reassign
> to $asm_file for clarity) for the .S?
>
This isn't actually a copy-pasto problem. I am using macro-source-path.c 
because I only need a .c file with a main function and the label, so I 
decided to go with a file that was already there. Do you think I should 
have a .c file anyway, or should I just document this for v3?

Cheers,
Bruno
  

On 2022-10-17 10:54, Bruno Larsen wrote:
> Hi Simon,
> 
> thanks for the review. I've fixed everything else you mentioned, but:
> 
> On 17/10/2022 14:32, Simon Marchi wrote:
>>> +#https://github.com/llvm/llvm-project/issues/54506
>>> +
>>> +load_lib dwarf.exp
>>> +
>>> +if {![dwarf2_support]} {
>>> +    return 0
>>> +}
>>> +if {![test_compiler_info gcc-*-*]} {
>>> +    untested "dwarf assembler needs GCC"
>>> +}
>>> +
>>> +standard_testfile .S
>>> +
>>> +set srcfile macro-source-path.c
>> copy-pasto.  Actually, can you use:
>>
>>    standard_testfile .c .S
>>
>> and then use $srcfile for the .c, and $srcfile2 (which you can reassign
>> to $asm_file for clarity) for the .S?
>>
> This isn't actually a copy-pasto problem. I am using macro-source-path.c because I only need a .c file with a main function and the label, so I decided to go with a file that was already there. Do you think I should have a .c file anyway, or should I just document this for v3?

I prefer not to share test files randomly between tests, for
consistency, clarity, and so there aren't surprises when you modify a
test.

Simon