[PATCH 0/4] gas: Dwarf generation adjustments

[PATCH 0/4] gas: Dwarf generation adjustments

[Jan Beulich]

While working on the partial (see there) fix contained in patch 1 I've
noticed a few more things which the remaining patches here attempt to
improve.

1: Dwarf: improve debug info generation from .irp and alike blocks
2: Dwarf5: drop dead code
3: Dwarf5: adjust .debug_line file 0 checking
4: Dwarf5: re-use file 0 line string table entry when faking file 0

Jan

[PATCH 1/4] gas/Dwarf: improve debug info generation from .irp and alike blocks

[Jan Beulich]

Tying the bumping of the logical line number to reading from the
original source file looks wrong: Upon finishing of the processing of an
sb the original values will be restored anyway. Yet without bumping the
line counter uses of .line inside e.g. an .irp construct won't have the
intended effect: Such uses may be necessary to ensure proper debug info
is emitted in particular when switching sections inside the .irp body,
as dwarf2_gen_line_info() would bail without doing anything when it
finds the line number unchanged from what it saw last.
---
Of course it would be even better if proper debug info was generated
without the need to use .line (and .file; see below), but I couldn't
figure a sensible way to achieve that, as it would require expand_irp()
(and likely others) to insert markers (when starting the next iteration)
for e.g. the scrubber to consume. Since .line isn't a universally
available directive, simply inserting an instance of that is not an
option, for example.

As to .file being required for .line to have any effect (see as_where()
for why this is) - that's not documented anywhere afaics, and hence I
wonder whether this isn't another bug.

dwarf2_gen_line_info() bailing when finding the line number unchanged is
a problem elsewhere as well, I think: Shouldn't it also take into
consideration whether now_{,sub}seg have changed? After all
.debug_aranges and .debug_rnglists are populated from the collected info
as well, and with them incomplete analysis or debugging tools won't have
a complete view of the sections a source file contributes to. The issue
with it not doing so can actually be seen when removing one of the .nop
in the new testcase ...

Further I wonder about line numbers for directives which actually
generate code: .nop is documented to have debug info generated, whereas
.nops is explicitly documented to not have this effect (for whatever
reason). Other directives have nothing stated, and most don't generate
debug info - not even ones specifically there to generate instructions,
like e.g. Arm's .inst (but unlike RISC-V's .insn).

--- a/gas/input-scrub.c
+++ b/gas/input-scrub.c
@@ -429,11 +429,10 @@ void
 bump_line_counters (void)
 {
   if (sb_index == (size_t) -1)
-    {
-      ++physical_input_line;
-      if (logical_input_line != -1u)
-	++logical_input_line;
-    }
+    ++physical_input_line;
+
+  if (logical_input_line != -1u)
+    ++logical_input_line;
 }
 \f
 /* Tells us what the new logical line number and file are.
--- /dev/null
+++ b/gas/testsuite/gas/elf/dwarf-5-irp.d
@@ -0,0 +1,80 @@
+#as: --gdwarf-5
+#name: line number entries for section changes inside .irp
+#readelf: -W -wlrR
+# The cr16 crx ft32 mn10* msp430 nds32* and rl78 targets do not evaluate the subtraction of symbols at assembly time.
+# The bfin target does not allow .subsection with an equated symbol as operand.
+# The d30v target emits sufficiently different debug info, apparently also covering padding it inserts.
+# The riscv targets do not support the subtraction of symbols.
+#xfail: bfin-* cr16-* crx-* d30v-* ft32-* mn10*-* msp430-* nds32*-* riscv*-* rl78-*
+
+Raw dump of debug contents .*
+#...
+ Line Number Statements:
+.*Extended opcode 2: .*
+.*Special opcode .* and Line by 2 to 3
+.*Set File Name to entry 2 .*
+.*Advance Line by 15 to 18
+.*Special opcode .* and Line by 0 to 18
+.*Special opcode .* and Line by 1 to 19
+.*Special opcode .* and Line by -1 to 18
+.*Special opcode .* and Line by 1 to 19
+.*Special opcode .* and Line by -1 to 18
+.*Special opcode .* and Line by 1 to 19
+.*Set File Name to entry 3 .*
+.*Advance Line by 9 to 28
+.*Special opcode .* and Line by 0 to 28
+.*Special opcode .* and Line by 1 to 29
+.*Special opcode .* and Line by -1 to 28
+.*Special opcode .* and Line by 1 to 29
+.*Special opcode .* and Line by -1 to 28
+.*Special opcode .* and Line by 1 to 29
+.*Advance PC by .*
+.*Extended opcode 1: End of Sequence
+
+.*Set File Name to entry 4 .*
+.*Extended opcode 2: .*
+.*Special opcode .* and Line by 8 to 9
+.*Special opcode .* and Line by 1 to 10
+.*Advance PC by .*
+.*Extended opcode 1: End of Sequence
+
+.*Set File Name to entry 4 .*
+.*Extended opcode 2: .*
+.*Special opcode .* and Line by 8 to 9
+.*Special opcode .* and Line by 1 to 10
+.*Advance PC by .*
+.*Extended opcode 1: End of Sequence
+
+.*Set File Name to entry 4 .*
+.*Extended opcode 2: .*
+.*Special opcode .* and Line by 8 to 9
+.*Special opcode .* and Line by 1 to 10
+.*Advance PC by .*
+.*Extended opcode 1: End of Sequence
+
+
+Contents of the \.debug_aranges section:
+
+  Length: .*
+  Version: +2
+  Offset into \.debug_info: .*
+  Pointer Size: +[248]
+  Segment Size: +0
+
+    Address +Length
+    0+ [0-9a-f]+ ?
+    0+ [0-9a-f]+ ?
+    0+ [0-9a-f]+ ?
+    0+ [0-9a-f]+ ?
+    0+ 0+ ?
+
+Contents of the \.debug_rnglists section:
+
+    Offset +Begin +End
+    [0-9a-f]+ 0+ [0-9a-f]+ ?
+    [0-9a-f]+ 0+ [0-9a-f]+ ?
+    [0-9a-f]+ 0+ [0-9a-f]+ ?
+    [0-9a-f]+ 0+ [0-9a-f]+ ?
+    [0-9a-f]+ <End of list>
+
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/elf/dwarf-5-irp.s
@@ -0,0 +1,31 @@
+	.text
+_start:
+	.nop
+
+	.irp n, ab, ij, xy
+	.file "irp.s"
+	.line 7
+	.section .text.\n, "ax"
+	.nop
+	.nop
+	.endr
+
+	.text
+
+	.irpc n, 123
+	.file "irpc.s"
+	.line 16
+	.subsection \n
+	.nop
+	.nop
+	.endr
+
+	n = 9
+	.rept 3
+	.file "rept.s"
+	.line 26
+	.subsection n
+	.nop
+	.nop
+	n = n - 1
+	.endr
--- a/gas/testsuite/gas/elf/elf.exp
+++ b/gas/testsuite/gas/elf/elf.exp
@@ -306,6 +306,7 @@ if { [is_elf_format] } then {
     run_dump_test "dwarf-4-cu" $dump_opts
     run_dump_test "dwarf-5-cu" $dump_opts
     run_dump_test "dwarf-5-nop-for-line-table" $dump_opts
+    run_dump_test "dwarf-5-irp" $dump_opts
     run_dump_test "pr25917"
     run_dump_test "bss"
     run_dump_test "bad-bss"

[PATCH 2/4] gas/Dwarf5: drop dead code

[Jan Beulich]

Commit 3417bfca676f ("GAS: DWARF-5: Ensure that the 0'th entry in the
directory table contains the current... ") added a "dwarf_level < 5"
check to out_dir_and_file_list(). This rendered dead that branch of the
construct, due to the enclosing if()'s "DWARF2_LINE_VERSION >= 5".
Delete that code as well as the corresponding part of the comment.

While there also drop a redundant "dirs != NULL": "dirs" will always be
non-NULL when dirs_in_use is not zero.

--- a/gas/dwarf2dbg.c
+++ b/gas/dwarf2dbg.c
@@ -2170,18 +2170,10 @@ out_dir_and_file_list (segT line_seg, in
       line_str_seg->entsize = 1;
 
       /* DWARF5 uses slot zero, but that is only set explicitly
-	 using a .file 0 directive.  If that isn't used, but dir
-	 one is used, then use that as main file directory.
-	 Otherwise use pwd as main file directory.  */
-      if (dirs_in_use > 0 && dirs != NULL && dirs[0] != NULL)
+	 using a .file 0 directive.  Otherwise use pwd as main file
+	 directory.  */
+      if (dirs_in_use > 0 && dirs[0] != NULL)
 	dir = remap_debug_filename (dirs[0]);
-      else if (dirs_in_use > 1
-	       && dirs != NULL
-	       && dirs[1] != NULL
-	       /* DWARF-5 directory tables expect dir[0] to be the same as
-		  DW_AT_comp_dir, which is the same as pwd.  */
-	       && dwarf_level < 5)
-	dir = remap_debug_filename (dirs[1]);
       else
 	dir = remap_debug_filename (getpwd ());
 

[PATCH 3/4] gas/Dwarf5: adjust .debug_line file 0 checking

[Jan Beulich]

First of all when a table entry has a NULL filename, the two inner if()s
are better done the other way around: The 2nd doesn't depend on what the
first does. This then renders redundant half of the conditions of the
other if() and clarifies that subsequently only entry 0 is dealt with
(indicating that part of the comment was wrong). Finally for there to be
a usable name in slot 1, files_in_use needs to be larger than 1 and slot
1's (rather than slot 0's) name needs to be non-NULL.

--- a/gas/dwarf2dbg.c
+++ b/gas/dwarf2dbg.c
@@ -2271,11 +2271,15 @@ out_dir_and_file_list (segT line_seg, in
 
       if (files[i].filename == NULL)
 	{
-	  /* Prevent a crash later, particularly for file 1.  DWARF5
-	     uses slot zero, but that is only set explicitly using a
-	     .file 0 directive.  If that isn't used, but file 1 is,
-	     then use that as main file name.  */
-	  if (DWARF2_LINE_VERSION >= 5 && i == 0 && files_in_use >= 1 && files[0].filename == NULL)
+	  if (DWARF2_LINE_VERSION < 5 || i != 0)
+	    {
+	      as_bad (_("unassigned file number %ld"), (long) i);
+	      continue;
+	    }
+	  /* DWARF5 uses slot zero, but that is only set explicitly using
+	     a .file 0 directive.  If that isn't used, but file 1 is, then
+	     use that as main file name.  */
+	  if (files_in_use > 1 && files[1].filename != NULL)
 	    {
 	      files[0].filename = files[1].filename;
 	      files[0].dir = files[1].dir;
@@ -2284,12 +2288,7 @@ out_dir_and_file_list (segT line_seg, in
 		  files[0].md5[j] = files[1].md5[j];
 	    }
 	  else
-	    files[i].filename = "";
-	  if (DWARF2_LINE_VERSION < 5 || i != 0)
-	    {
-	      as_bad (_("unassigned file number %ld"), (long) i);
-	      continue;
-	    }
+	    files[0].filename = "";
 	}
 
       fullfilename = DWARF2_FILE_NAME (files[i].filename,

[PATCH 4/4] gas/Dwarf5: re-use file 0 line string table entry when faking file 0

[Jan Beulich]

No need to emit the same string a 2nd time for file 1 in this case.
---
An alternative might be to, just like is already the case for
directories, pull file 0 handling out of the loop.

--- a/gas/dwarf2dbg.c
+++ b/gas/dwarf2dbg.c
@@ -2137,7 +2137,7 @@ out_dir_and_file_list (segT line_seg, in
   bool emit_timestamps = true;
   bool emit_filesize = true;
   segT line_str_seg = NULL;
-  symbolS *line_strp;
+  symbolS *line_strp, *file0_strp = NULL;
 
   /* Output the Directory Table.  */
   if (DWARF2_LINE_VERSION >= 5)
@@ -2301,9 +2301,17 @@ out_dir_and_file_list (segT line_seg, in
 	}
       else
 	{
-	  line_strp = add_line_strp (line_str_seg, fullfilename);
+	  if (!file0_strp)
+	    line_strp = add_line_strp (line_str_seg, fullfilename);
+	  else
+	    line_strp = file0_strp;
 	  subseg_set (line_seg, 0);
 	  TC_DWARF2_EMIT_OFFSET (line_strp, sizeof_offset);
+	  if (i == 0 && files_in_use > 1
+	      && files[0].filename == files[1].filename)
+	    file0_strp = line_strp;
+	  else
+	    file0_strp = NULL;
 	}
 
       /* Directory number.  */

Re: [PATCH 2/4] gas/Dwarf5: drop dead code

[Nick Clifton]

Hi Jan,

> --- a/gas/dwarf2dbg.c
> +++ b/gas/dwarf2dbg.c
> @@ -2170,18 +2170,10 @@ out_dir_and_file_list (segT line_seg, in
>         line_str_seg->entsize = 1;
>   
>         /* DWARF5 uses slot zero, but that is only set explicitly
> -	 using a .file 0 directive.  If that isn't used, but dir
> -	 one is used, then use that as main file directory.
> -	 Otherwise use pwd as main file directory.  */
> -      if (dirs_in_use > 0 && dirs != NULL && dirs[0] != NULL)
> +	 using a .file 0 directive.  Otherwise use pwd as main file
> +	 directory.  */
> +      if (dirs_in_use > 0 && dirs[0] != NULL)
>   	dir = remap_debug_filename (dirs[0]);
> -      else if (dirs_in_use > 1
> -	       && dirs != NULL
> -	       && dirs[1] != NULL
> -	       /* DWARF-5 directory tables expect dir[0] to be the same as
> -		  DW_AT_comp_dir, which is the same as pwd.  */
> -	       && dwarf_level < 5)
> -	dir = remap_debug_filename (dirs[1]);
>         else
>   	dir = remap_debug_filename (getpwd ());

Approved - please apply.

Cheers
   Nick

Re: [PATCH 3/4] gas/Dwarf5: adjust .debug_line file 0 checking

[Nick Clifton]

Hi Jan,

> First of all when a table entry has a NULL filename, the two inner if()s
> are better done the other way around: The 2nd doesn't depend on what the
> first does. This then renders redundant half of the conditions of the
> other if() and clarifies that subsequently only entry 0 is dealt with
> (indicating that part of the comment was wrong). Finally for there to be
> a usable name in slot 1, files_in_use needs to be larger than 1 and slot
> 1's (rather than slot 0's) name needs to be non-NULL.

Approved - please apply.

Cheers
   Nick

Re: [PATCH 4/4] gas/Dwarf5: re-use file 0 line string table entry when faking file 0

[Nick Clifton]

Hi Jan,

> No need to emit the same string a 2nd time for file 1 in this case.

Approved - please apply.

> An alternative might be to, just like is already the case for
> directories, pull file 0 handling out of the loop.

If you want to, then please submit another patch.  But I am happy
with the way things are at the moment.

Cheers
   Nick

Re: [PATCH 1/4] gas/Dwarf: improve debug info generation from .irp and alike blocks

[Nick Clifton]

Hi Jan,

> As to .file being required for .line to have any effect (see as_where()
> for why this is) - that's not documented anywhere afaics, and hence I
> wonder whether this isn't another bug.

Is that right ?  Looking at the code, it seems to me that either
physical_file_name or logical_file_name would have to be set any time a
.line directive is encountered.  Is there a sceanario where this does
not happen ?


> dwarf2_gen_line_info() bailing when finding the line number unchanged is
> a problem elsewhere as well, I think: Shouldn't it also take into
> consideration whether now_{,sub}seg have changed? After all
> .debug_aranges and .debug_rnglists are populated from the collected info
> as well, and with them incomplete analysis or debugging tools won't have
> a complete view of the sections a source file contributes to. The issue
> with it not doing so can actually be seen when removing one of the .nop
> in the new testcase ...

Agreed - this should be examined and fixed.

> Further I wonder about line numbers for directives which actually
> generate code: .nop is documented to have debug info generated, whereas
> .nops is explicitly documented to not have this effect (for whatever
> reason). Other directives have nothing stated, and most don't generate
> debug info - not even ones specifically there to generate instructions,
> like e.g. Arm's .inst (but unlike RISC-V's .insn).

The target specific instruction directives definitely should generate DWARF
info, so if they don't it is a bug.

Anyway, patch, and testcase, approved - please apply.

Cheers
   Nick

Re: [PATCH 1/4] gas/Dwarf: improve debug info generation from .irp and alike blocks

[Jan Beulich]

On 22.03.2022 15:18, Nick Clifton wrote:
>> As to .file being required for .line to have any effect (see as_where()
>> for why this is) - that's not documented anywhere afaics, and hence I
>> wonder whether this isn't another bug.
> 
> Is that right ?  Looking at the code, it seems to me that either
> physical_file_name or logical_file_name would have to be set any time a
> .line directive is encountered.  Is there a sceanario where this does
> not happen ?

Well, yes, from my testing removing the .file from the new testcase
renders the .line there meaningless (as far as debug info generation
goes). For as_where() to return logical_input_{file,line},
logical_input_file needs to be non-NULL; as long as it falls back to
as_where_physical(), the checking that file/line have actually
changed would prevent new information to be recorded. But of course if
the problem described below was fixed, I think the problem here would
vanish as a side effect (except that actual line numbers recorded may
then still be sub-optimal).

>> dwarf2_gen_line_info() bailing when finding the line number unchanged is
>> a problem elsewhere as well, I think: Shouldn't it also take into
>> consideration whether now_{,sub}seg have changed? After all
>> .debug_aranges and .debug_rnglists are populated from the collected info
>> as well, and with them incomplete analysis or debugging tools won't have
>> a complete view of the sections a source file contributes to. The issue
>> with it not doing so can actually be seen when removing one of the .nop
>> in the new testcase ...
> 
> Agreed - this should be examined and fixed.

Any thoughts regarding the mentioned checking of now_seg / now_subseg
there, to deal with this?

>> Further I wonder about line numbers for directives which actually
>> generate code: .nop is documented to have debug info generated, whereas
>> .nops is explicitly documented to not have this effect (for whatever
>> reason). Other directives have nothing stated, and most don't generate
>> debug info - not even ones specifically there to generate instructions,
>> like e.g. Arm's .inst (but unlike RISC-V's .insn).
> 
> The target specific instruction directives definitely should generate DWARF
> info, so if they don't it is a bug.

Okay, so at least Arm would seem to need fixing. I'll see if I can get
to this.

Do you have an idea why .nops is explicitly documented to not generate
debug info? And can you provide any insight on why e.g. .byte wouldn't
want to have line number info generated when used in an executable
section?

> Anyway, patch, and testcase, approved - please apply.

Thanks (including the other approvals).

Jan