Broken GAS DWARF tests

Broken GAS DWARF tests

[Nick Clifton]

Hi Guys,

  Earlier today I checked in a small patch to fix a problem with the assembler.
  If no -g option is provided on the command line it would default to generating
  DWARF level 0 line tables.  Which is invalid of course since there is no DWARF
  level 0.  So I applied a small patch to change the default to 4.  And then I
  forgot the cardinal rule and did not test the patch before checking it in.
  *sigh*

  Of course it broke a couple of the assembler tests.  So now I am actually testing
  a local fix before checking that in.  My apologese for any inconvenience this
  oversight has caused.

Cheers
  Nick

Re: Broken GAS DWARF tests

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

On Thu, Jul 30, 2020 at 2:46 AM Nick Clifton via Binutils
<binutils@sourceware.org> wrote:
>
> Hi Guys,
>
>   Earlier today I checked in a small patch to fix a problem with the assembler.
>   If no -g option is provided on the command line it would default to generating
>   DWARF level 0 line tables.  Which is invalid of course since there is no DWARF
>   level 0.  So I applied a small patch to change the default to 4.  And then I
>   forgot the cardinal rule and did not test the patch before checking it in.
>   *sigh*
>
>   Of course it broke a couple of the assembler tests.  So now I am actually testing
>   a local fix before checking that in.  My apologese for any inconvenience this
>   oversight has caused.
>

Here is the one I am using for x86.

-- 
H.J.

[-- Attachment #2: g.patch --]
[-- Type: text/x-patch, Size: 5613 bytes --]

diff --git a/binutils/testsuite/binutils-all/i386/compressed-1a.d b/binutils/testsuite/binutils-all/i386/compressed-1a.d
index 70b13cee5d..bf04974a89 100644
--- a/binutils/testsuite/binutils-all/i386/compressed-1a.d
+++ b/binutils/testsuite/binutils-all/i386/compressed-1a.d
@@ -1,6 +1,6 @@
 #PROG: objcopy
 #source: compressed-1.s
-#as: --32 --compress-debug-sections
+#as: --32 --compress-debug-sections --gdwarf-3
 #objcopy:
 #readelf: -w
 #name: objcopy on compressed debug sections
diff --git a/binutils/testsuite/binutils-all/i386/compressed-1b.d b/binutils/testsuite/binutils-all/i386/compressed-1b.d
index 9ed82caf60..6e45e552fc 100644
--- a/binutils/testsuite/binutils-all/i386/compressed-1b.d
+++ b/binutils/testsuite/binutils-all/i386/compressed-1b.d
@@ -1,6 +1,6 @@
 #PROG: strip
 #source: compressed-1.s
-#as: --32 -mx86-used-note=no
+#as: --32 -mx86-used-note=no --gdwarf-3
 #strip:
 #readelf: -S --wide
 #name: strip on uncompressed debug sections
diff --git a/binutils/testsuite/binutils-all/i386/compressed-1c.d b/binutils/testsuite/binutils-all/i386/compressed-1c.d
index dc96dd7dcf..1e1a18e778 100644
--- a/binutils/testsuite/binutils-all/i386/compressed-1c.d
+++ b/binutils/testsuite/binutils-all/i386/compressed-1c.d
@@ -1,6 +1,6 @@
 #PROG: strip
 #source: compressed-1.s
-#as: --32 --compress-debug-sections -mx86-used-note=no
+#as: --32 --compress-debug-sections -mx86-used-note=no --gdwarf-3
 #strip:
 #readelf: -S --wide
 #name: strip on compressed debug sections
diff --git a/binutils/testsuite/binutils-all/x86-64/compressed-1a.d b/binutils/testsuite/binutils-all/x86-64/compressed-1a.d
index 3c8eb54502..c4898d7748 100644
--- a/binutils/testsuite/binutils-all/x86-64/compressed-1a.d
+++ b/binutils/testsuite/binutils-all/x86-64/compressed-1a.d
@@ -1,6 +1,6 @@
 #PROG: objcopy
 #source: compressed-1.s
-#as: --64 --compress-debug-sections
+#as: --64 --compress-debug-sections --gdwarf-3
 #objcopy:
 #readelf: -w
 #name: objcopy on compressed debug sections
diff --git a/binutils/testsuite/binutils-all/x86-64/compressed-1b.d b/binutils/testsuite/binutils-all/x86-64/compressed-1b.d
index c041a53260..517c814539 100644
--- a/binutils/testsuite/binutils-all/x86-64/compressed-1b.d
+++ b/binutils/testsuite/binutils-all/x86-64/compressed-1b.d
@@ -1,6 +1,6 @@
 #PROG: strip
 #source: compressed-1.s
-#as: --64 -mx86-used-note=no
+#as: --64 -mx86-used-note=no --gdwarf-3
 #strip:
 #readelf: -S --wide
 #name: strip on uncompressed debug sections
diff --git a/binutils/testsuite/binutils-all/x86-64/compressed-1c.d b/binutils/testsuite/binutils-all/x86-64/compressed-1c.d
index 2039cd676e..ba692bdb85 100644
--- a/binutils/testsuite/binutils-all/x86-64/compressed-1c.d
+++ b/binutils/testsuite/binutils-all/x86-64/compressed-1c.d
@@ -1,6 +1,6 @@
 #PROG: strip
 #source: compressed-1.s
-#as: --64 --compress-debug-sections -mx86-used-note=no
+#as: --64 --compress-debug-sections -mx86-used-note=no --gdwarf-3
 #strip:
 #readelf: -S --wide
 #name: strip on compressed debug sections
diff --git a/gas/testsuite/gas/elf/dwarf2-3.d b/gas/testsuite/gas/elf/dwarf2-3.d
index 594c9a6d97..62dbe54ce0 100644
--- a/gas/testsuite/gas/elf/dwarf2-3.d
+++ b/gas/testsuite/gas/elf/dwarf2-3.d
@@ -1,3 +1,4 @@
+#as: --gdwarf-3
 #readelf: -wl
 #name: DWARF2 3
 #xfail: ft32*-* h8300-*-*
diff --git a/gas/testsuite/gas/elf/dwarf2-5.d b/gas/testsuite/gas/elf/dwarf2-5.d
index 4e22e79c64..257bb55a01 100644
--- a/gas/testsuite/gas/elf/dwarf2-5.d
+++ b/gas/testsuite/gas/elf/dwarf2-5.d
@@ -1,4 +1,4 @@
-#as:
+#as: --gdwarf-3
 #readelf: -x.rodata -wlL
 #name: DWARF2 5
 # The am33 cr16 crx ft32 mn10 msp430 nds32 rl78 and rx targets do not evaluate the subtraction of symbols at assembly time.
diff --git a/gas/testsuite/gas/i386/dw2-compress-3a.d b/gas/testsuite/gas/i386/dw2-compress-3a.d
index c6de0d6f57..50a1e28fa8 100644
--- a/gas/testsuite/gas/i386/dw2-compress-3a.d
+++ b/gas/testsuite/gas/i386/dw2-compress-3a.d
@@ -1,5 +1,5 @@
 #source: dw2-compress-3.s
-#as: --compress-debug-sections --elf-stt-common=no
+#as: --compress-debug-sections --elf-stt-common=no --gdwarf-3
 #readelf: -w
 #name: DWARF2 debugging information 3 w/o STT_COMMON
 
diff --git a/gas/testsuite/gas/i386/dw2-compress-3b.d b/gas/testsuite/gas/i386/dw2-compress-3b.d
index a78e1e25e9..798d5532c3 100644
--- a/gas/testsuite/gas/i386/dw2-compress-3b.d
+++ b/gas/testsuite/gas/i386/dw2-compress-3b.d
@@ -1,5 +1,5 @@
 #source: dw2-compress-3.s
-#as: --compress-debug-sections --elf-stt-common=yes
+#as: --compress-debug-sections --elf-stt-common=yes --gdwarf-3
 #readelf: -w
 #name: DWARF2 debugging information 3 with STT_COMMON
 
diff --git a/gas/testsuite/gas/i386/dw2-compressed-3a.d b/gas/testsuite/gas/i386/dw2-compressed-3a.d
index b8b4434a51..e4fcf535b6 100644
--- a/gas/testsuite/gas/i386/dw2-compressed-3a.d
+++ b/gas/testsuite/gas/i386/dw2-compressed-3a.d
@@ -1,5 +1,5 @@
 #source: dw2-compress-3.s
-#as: --compress-debug-sections=zlib-gabi --elf-stt-common=no
+#as: --compress-debug-sections=zlib-gabi --elf-stt-common=no --gdwarf-3
 #readelf: -w
 #name: DWARF2 debugging information 3 with SHF_COMPRESSED w/o STT_COMMON
 
diff --git a/gas/testsuite/gas/i386/dw2-compressed-3b.d b/gas/testsuite/gas/i386/dw2-compressed-3b.d
index ac51238a96..37f6b7effb 100644
--- a/gas/testsuite/gas/i386/dw2-compressed-3b.d
+++ b/gas/testsuite/gas/i386/dw2-compressed-3b.d
@@ -1,5 +1,5 @@
 #source: dw2-compress-3.s
-#as: --compress-debug-sections=zlib-gabi --elf-stt-common=yes
+#as: --compress-debug-sections=zlib-gabi --elf-stt-common=yes --gdwarf-3
 #readelf: -w
 #name: DWARF2 debugging information 3 with SHF_COMPRESSED with STT_COMMON
 

Re: Broken GAS DWARF tests

[H.J. Lu]

On Thu, Jul 30, 2020 at 4:13 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Jul 30, 2020 at 2:46 AM Nick Clifton via Binutils
> <binutils@sourceware.org> wrote:
> >
> > Hi Guys,
> >
> >   Earlier today I checked in a small patch to fix a problem with the assembler.
> >   If no -g option is provided on the command line it would default to generating
> >   DWARF level 0 line tables.  Which is invalid of course since there is no DWARF
> >   level 0.  So I applied a small patch to change the default to 4.  And then I
> >   forgot the cardinal rule and did not test the patch before checking it in.
> >   *sigh*
> >
> >   Of course it broke a couple of the assembler tests.  So now I am actually testing
> >   a local fix before checking that in.  My apologese for any inconvenience this
> >   oversight has caused.
> >
>
> Here is the one I am using for x86.
>

I am checking in x86 changes.

-- 
H.J.

Re: Broken GAS DWARF tests

[Nick Clifton]

Hi Guys.

>   Of course it broke a couple of the assembler tests.  So now I am actually testing
>   a local fix before checking that in.  My apologese for any inconvenience this
>   oversight has caused.

Ok, I have checked in the patch below to fix my snafu.

Cheers
  Nick

gas/ChangeLog
2020-07-30  Nick Clifton  <nickc@redhat.com>

	* as.c (dwatf_level): Default to level 3 in case version is not
	set on the command line.

diff --git a/gas/as.c b/gas/as.c
index c2da78870e..e4cf1364cb 100644
--- a/gas/as.c
+++ b/gas/as.c
@@ -102,8 +102,9 @@ int verbose = 0;
    MD_AFTER_PARSE_ARGS.  */
 int flag_dwarf_cie_version = -1;
 
-/* The maximum level of DWARF DEBUG information we should manufacture.  */
-unsigned int dwarf_level = 4;
+/* The maximum level of DWARF DEBUG information we should manufacture.
+   The default is 3 unless changes by a command line option.  */
+unsigned int dwarf_level = 3;
 
 #if defined OBJ_ELF || defined OBJ_MAYBE_ELF
 int flag_use_elf_stt_common = DEFAULT_GENERATE_ELF_STT_COMMON;

Re: Broken GAS DWARF tests

[Andreas Schwab]

On Jul 30 2020, Nick Clifton via Binutils wrote:

> diff --git a/gas/as.c b/gas/as.c
> index c2da78870e..e4cf1364cb 100644
> --- a/gas/as.c
> +++ b/gas/as.c
> @@ -102,8 +102,9 @@ int verbose = 0;
>     MD_AFTER_PARSE_ARGS.  */
>  int flag_dwarf_cie_version = -1;
>  
> -/* The maximum level of DWARF DEBUG information we should manufacture.  */
> -unsigned int dwarf_level = 4;
> +/* The maximum level of DWARF DEBUG information we should manufacture.
> +   The default is 3 unless changes by a command line option.  */

s/changes/changed/

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: Broken GAS DWARF tests

[Tom Tromey]

>>>>> "Nick" == Nick Clifton via Binutils <binutils@sourceware.org> writes:

Nick> gas/ChangeLog
Nick> 2020-07-30  Nick Clifton  <nickc@redhat.com>

Nick> 	* as.c (dwatf_level): Default to level 3 in case version is not
Nick> 	set on the command line.

I understand not wanting to pick DWARF 5 as a default, since it still
isn't fully supported in gdb; but why not DWARF 4 as the default?

Tom

Re: Broken GAS DWARF tests

[Nick Clifton]

Hi Tom,

> Nick> 	* as.c (dwatf_level): Default to level 3 in case version is not
> Nick> 	set on the command line.
> 
> I understand not wanting to pick DWARF 5 as a default, since it still
> isn't fully supported in gdb; but why not DWARF 4 as the default?

Simplicity.  Setting the default to 4 broke lots of tests (in the assembler
and binutils directories).  Setting the default to 3 does not break the tests.

Plus, and this is very minor, version 4 DWARF line number tables are 
actually bigger than version 3 tables, because of the addition of the
Maximum Ops Per Instruction field in the table header.

But if there is a consensus that version 4 is the way to go, then I can
change the default and update the tests.

Cheers
  Nick

Re: Broken GAS DWARF tests

[Mark Wielaard]

On Thu, Jul 30, 2020 at 04:21:17PM +0100, Nick Clifton via Binutils wrote:
> Hi Tom,
> 
> > Nick> 	* as.c (dwatf_level): Default to level 3 in case version is not
> > Nick> 	set on the command line.
> > 
> > I understand not wanting to pick DWARF 5 as a default, since it still
> > isn't fully supported in gdb; but why not DWARF 4 as the default?
> 
> Simplicity.  Setting the default to 4 broke lots of tests (in the assembler
> and binutils directories).  Setting the default to 3 does not break the tests.
> 
> Plus, and this is very minor, version 4 DWARF line number tables are 
> actually bigger than version 3 tables, because of the addition of the
> Maximum Ops Per Instruction field in the table header.

This is most likely compensated by the fact that for DWARF4 we loose a
relocation in the CU DIE header for DW_AT_high_pc. We could save a few
bytes if we would be able to express the difference between low_pc and
high_pc as uleb128 (maybe we can simply use emit_leb128_expr?).

> But if there is a consensus that version 4 is the way to go, then I can
> change the default and update the tests.

It might be a good idea to upgrade to version 4 for the next binutils
release. DWARF4 was officially published 10 years ago (and DWARF5 3
years ago).

Cheers,

Mark

gas: Use udata for DW_AT_high_pc when emitting DWARF4 (Was: Broken GAS DWARF tests)

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Mon, Aug 03, 2020 at 03:11:09PM +0200, Mark Wielaard wrote:
> On Thu, Jul 30, 2020 at 04:21:17PM +0100, Nick Clifton via Binutils wrote:
> > Plus, and this is very minor, version 4 DWARF line number tables are 
> > actually bigger than version 3 tables, because of the addition of the
> > Maximum Ops Per Instruction field in the table header.
> 
> This is most likely compensated by the fact that for DWARF4 we loose a
> relocation in the CU DIE header for DW_AT_high_pc. We could save a few
> bytes if we would be able to express the difference between low_pc and
> high_pc as uleb128 (maybe we can simply use emit_leb128_expr?).

This seems to work as intended and is indeed a lot shorter (before
this patch we always used either data4 (32bits) or data8 (64bits).

Cheers,

Mark

[-- Attachment #2: 0001-gas-Use-udata-for-DW_AT_high_pc-when-emitting-DWARF4.patch --]
[-- Type: text/x-diff, Size: 3145 bytes --]

From 0b202965ae7aa2ed2ce3d3db29d806ce0f1da0d4 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Mon, 3 Aug 2020 22:02:24 +0200
Subject: [PATCH] gas: Use udata for DW_AT_high_pc when emitting DWARF4

For DWARF4 DW_AT_high_pc can be expressed as constant offset from
DW_AT_low_pc which saves a relocation. Use DW_FORM_udate (uleb128)
to keep the constant value as small as possible.

gas/ChangeLog:

       * dwarf2dbg.c (out_debug_abbrev): When DWARF2_VERSION >= 4, use
       DW_FORM_udata for DW_AT_high_pc.
       (out_debug_info): Use emit_leb128_expr for DW_AT_high_pc, when
       DWARF2_VERSION >= 4.
       * read.c (emit_leb128_exp): No longer static.
       * read.h (emit_leb128_exp): Define.
---
 gas/ChangeLog   | 9 +++++++++
 gas/dwarf2dbg.c | 8 +++++---
 gas/read.c      | 2 +-
 gas/read.h      | 1 +
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/gas/ChangeLog b/gas/ChangeLog
index 8dee8190b0..ff2f762ed7 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,12 @@
+2020-08-04  Mark Wielaard  <mark@klomp.org>
+
+	* dwarf2dbg.c (out_debug_abbrev): When DWARF2_VERSION >= 4, use
+	DW_FORM_udata for DW_AT_high_pc.
+	(out_debug_info): Use emit_leb128_expr for DW_AT_high_pc, when
+	DWARF2_VERSION >= 4.
+	* read.c (emit_leb128_exp): No longer static.
+	* read.h (emit_leb128_exp): Define.
+
 2020-07-30  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR gas/26305
diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
index 69955fea5b..64333aa1a5 100644
--- a/gas/dwarf2dbg.c
+++ b/gas/dwarf2dbg.c
@@ -2427,8 +2427,7 @@ out_debug_abbrev (segT abbrev_seg,
       if (DWARF2_VERSION < 4)
 	out_abbrev (DW_AT_high_pc, DW_FORM_addr);
       else
-	out_abbrev (DW_AT_high_pc, (sizeof_address == 4
-				    ? DW_FORM_data4 : DW_FORM_data8));
+	out_abbrev (DW_AT_high_pc, DW_FORM_udata);
     }
   else
     {
@@ -2497,7 +2496,10 @@ out_debug_info (segT info_seg, segT abbrev_seg, segT line_seg, segT ranges_seg,
 	}
       exp.X_add_symbol = all_segs->text_end;
       exp.X_add_number = 0;
-      emit_expr (&exp, sizeof_address);
+      if (DWARF2_VERSION < 4)
+	emit_expr (&exp, sizeof_address);
+      else
+	emit_leb128_expr (&exp, 0);
     }
   else
     {
diff --git a/gas/read.c b/gas/read.c
index 8f93c2ba2b..f192cc16d5 100644
--- a/gas/read.c
+++ b/gas/read.c
@@ -5138,7 +5138,7 @@ output_big_leb128 (char *p, LITTLENUM_TYPE *bignum, unsigned int size, int sign)
 /* Generate the appropriate fragments for a given expression to emit a
    leb128 value.  SIGN is 1 for sleb, 0 for uleb.  */
 
-static void
+void
 emit_leb128_expr (expressionS *exp, int sign)
 {
   operatorT op = exp->X_op;
diff --git a/gas/read.h b/gas/read.h
index 502f3b6f2d..ffcdbb69a7 100644
--- a/gas/read.h
+++ b/gas/read.h
@@ -132,6 +132,7 @@ extern void emit_expr_with_reloc (expressionS *exp, unsigned int nbytes,
 				  TC_PARSE_CONS_RETURN_TYPE);
 extern void emit_expr_fix (expressionS *, unsigned int, fragS *, char *,
 			   TC_PARSE_CONS_RETURN_TYPE);
+extern void emit_leb128_expr (expressionS *, int);
 extern void equals (char *, int);
 extern void float_cons (int);
 extern void ignore_rest_of_line (void);
-- 
2.20.1

Re: gas: Use udata for DW_AT_high_pc when emitting DWARF4 (Was: Broken GAS DWARF tests)

[Alan Modra]

On Mon, Aug 03, 2020 at 10:11:27PM +0200, Mark Wielaard wrote:
>        * dwarf2dbg.c (out_debug_abbrev): When DWARF2_VERSION >= 4, use
>        DW_FORM_udata for DW_AT_high_pc.
>        (out_debug_info): Use emit_leb128_expr for DW_AT_high_pc, when
>        DWARF2_VERSION >= 4.
>        * read.c (emit_leb128_exp): No longer static.
>        * read.h (emit_leb128_exp): Define.

OK, looks good.  Note that there have been problems in the past
resolving leb128 expressions (eg. see 38cf168be581), so please keep an
eye out for regressions.

-- 
Alan Modra
Australia Development Lab, IBM

Re: gas: Use udata for DW_AT_high_pc when emitting DWARF4 (Was: Broken GAS DWARF tests)

[Fangrui Song]

On 2020-08-04, Alan Modra via Binutils wrote:
>On Mon, Aug 03, 2020 at 10:11:27PM +0200, Mark Wielaard wrote:
>>        * dwarf2dbg.c (out_debug_abbrev): When DWARF2_VERSION >= 4, use
>>        DW_FORM_udata for DW_AT_high_pc.
>>        (out_debug_info): Use emit_leb128_expr for DW_AT_high_pc, when
>>        DWARF2_VERSION >= 4.
>>        * read.c (emit_leb128_exp): No longer static.
>>        * read.h (emit_leb128_exp): Define.
>
>OK, looks good.  Note that there have been problems in the past
>resolving leb128 expressions (eg. see 38cf168be581), so please keep an
>eye out for regressions.
>
>-- 
>Alan Modra
>Australia Development Lab, IBM

Using DWARF v3 attribute class 'address' for DW_AT_high_pc can save a
relocation on RISC-V in -mrelax mode

Re: gas: Use udata for DW_AT_high_pc when emitting DWARF4 (Was: Broken GAS DWARF tests)

[Mark Wielaard]

Hi,

On Wed, Aug 05, 2020 at 11:09:12PM -0700, Fangrui Song wrote:
> On 2020-08-04, Alan Modra via Binutils wrote:
> > On Mon, Aug 03, 2020 at 10:11:27PM +0200, Mark Wielaard wrote:
> > >        * dwarf2dbg.c (out_debug_abbrev): When DWARF2_VERSION >= 4, use
> > >        DW_FORM_udata for DW_AT_high_pc.
> > >        (out_debug_info): Use emit_leb128_expr for DW_AT_high_pc, when
> > >        DWARF2_VERSION >= 4.
> > >        * read.c (emit_leb128_exp): No longer static.
> > >        * read.h (emit_leb128_exp): Define.
> > 
> > OK, looks good.  Note that there have been problems in the past
> > resolving leb128 expressions (eg. see 38cf168be581), so please keep an
> > eye out for regressions.
> 
> Using DWARF v3 attribute class 'address' for DW_AT_high_pc can save a
> relocation on RISC-V in -mrelax mode

DW_FORM_addr is the default for DWARF2_VERSION < 4 (for both
DW_AT_low_pc and DW_AT_high_pc). Before this patch it was
DW_FORM_data4 (or DW_FORM_data8) for DW_AT_high_pc. Using data instead
of addr already did prevent a relocation (the data is an offset from
the low_pc address). Using DW_FORM_udata instead means the data
representation usually smaller (especially for 64bit arches).

How does -mrelax mode work to prevent having to generate a relocation
even when generating an address against a different section?

Thanks,

Mark