Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

[Mark Wielaard]

Hi gcc hackers, binutils hackers,

Nick, Jakub and I were discussing the gcc patch below and all the ways
it is wrong. Most things can be fixed in the spec. Like only passing
-gdwarf if we are generating DWARF and passing the right DWARF version
as -gdwarf-N for the version that gcc itself creates. But whether or
not we want gas to generate .debug_line info is a bit tricky. But when
giving -gdwarf-N gas will always try to generate a .debug_line section
and error out when there is already one.

Would it be possible to have something like the following in gas, so
that it doesn't try generating a .debug_line section if there already
is one, even when -gdwarf-N is given (unless the assembly also
contains .loc directives because that shows the user is really
confused)?

diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
index e4ba56d82ba..c0c09f4e9d0 100644
--- a/gas/dwarf2dbg.c
+++ b/gas/dwarf2dbg.c
@@ -2626,7 +2626,7 @@ dwarf2_init (void)
 
 
 /* Finish the dwarf2 debug sections.  We emit .debug.line if there
-   were any .file/.loc directives, or --gdwarf2 was given, or if the
+   were any .file/.loc directives, or --gdwarf2 was given, and if the
    file has a non-empty .debug_info section and an empty .debug_line
    section.  If we emit .debug_line, and the .debug_info section is
    empty, we also emit .debug_info, .debug_aranges and .debug_abbrev.
@@ -2650,9 +2650,16 @@ dwarf2_finish (void)
   empty_debug_line = line_seg == NULL || !seg_not_empty_p (line_seg);
 
   /* We can't construct a new debug_line section if we already have one.
-     Give an error.  */
+     Give an error if we have seen any .loc, otherwise trust the user
+     knows what they are doing and want to generate the .debug_line
+     (and all other debug sections) themselves.  */
   if (all_segs && !empty_debug_line)
-    as_fatal ("duplicate .debug_line sections");
+    {
+      if (dwarf2_loc_directive_seen)
+	as_fatal ("duplicate .debug_line sections");
+      else
+	return;
+    }
 
   if ((!all_segs && emit_other_sections)
       || (!emit_other_sections && !empty_debug_line))

On Mon, Aug 24, 2020 at 02:56:58PM +0200, Mark Wielaard wrote:
> This is needed to get DWARF version 5 .debug_line tables.
> It is also obviously wrong. It needs a check for whether as supports
> --gdwarf-<version> for all versions we support and it should only
> be added when generating DWARF debug information for the specific
> DWARF version we are generating.
> 
> It also needs some fixes to binutils, to make sure the line table is
> generated correctly:
> https://sourceware.org/pipermail/binutils/2020-August/112685.html
> And to make sure it can read the generated line table itself:
> https://sourceware.org/pipermail/binutils/2020-August/112966.html
> ---
>  gcc/gcc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/gcc/gcc.c b/gcc/gcc.c
> index 10bc9881aed3..98b10e7cd154 100644
> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c
> @@ -1882,6 +1882,11 @@ init_spec (void)
>    }
>  #endif
>  
> +  static const char dv[] = "--gdwarf-5 ";
> +  obstack_grow (&obstack, dv, sizeof (dv) - 1);
> +  obstack_grow0 (&obstack, asm_spec, strlen (asm_spec));
> +  asm_spec = XOBFINISH (&obstack, const char *);
> +
>  #if defined LINK_EH_SPEC || defined LINK_BUILDID_SPEC || \
>      defined LINKER_HASH_STYLE
>  # ifdef LINK_BUILDID_SPEC
> -- 
> 2.18.4
> 

Re: Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

[H.J. Lu]

On Wed, Aug 26, 2020 at 2:38 PM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi gcc hackers, binutils hackers,
>
> Nick, Jakub and I were discussing the gcc patch below and all the ways
> it is wrong. Most things can be fixed in the spec. Like only passing
> -gdwarf if we are generating DWARF and passing the right DWARF version
> as -gdwarf-N for the version that gcc itself creates. But whether or
> not we want gas to generate .debug_line info is a bit tricky. But when
> giving -gdwarf-N gas will always try to generate a .debug_line section
> and error out when there is already one.
>
> Would it be possible to have something like the following in gas, so
> that it doesn't try generating a .debug_line section if there already
> is one, even when -gdwarf-N is given (unless the assembly also
> contains .loc directives because that shows the user is really
> confused)?
>
> diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
> index e4ba56d82ba..c0c09f4e9d0 100644
> --- a/gas/dwarf2dbg.c
> +++ b/gas/dwarf2dbg.c
> @@ -2626,7 +2626,7 @@ dwarf2_init (void)
>
>
>  /* Finish the dwarf2 debug sections.  We emit .debug.line if there
> -   were any .file/.loc directives, or --gdwarf2 was given, or if the
> +   were any .file/.loc directives, or --gdwarf2 was given, and if the
>     file has a non-empty .debug_info section and an empty .debug_line
>     section.  If we emit .debug_line, and the .debug_info section is
>     empty, we also emit .debug_info, .debug_aranges and .debug_abbrev.
> @@ -2650,9 +2650,16 @@ dwarf2_finish (void)
>    empty_debug_line = line_seg == NULL || !seg_not_empty_p (line_seg);
>
>    /* We can't construct a new debug_line section if we already have one.
> -     Give an error.  */
> +     Give an error if we have seen any .loc, otherwise trust the user
> +     knows what they are doing and want to generate the .debug_line
> +     (and all other debug sections) themselves.  */
>    if (all_segs && !empty_debug_line)
> -    as_fatal ("duplicate .debug_line sections");
> +    {
> +      if (dwarf2_loc_directive_seen)
> +       as_fatal ("duplicate .debug_line sections");
> +      else
> +       return;
> +    }
>
>    if ((!all_segs && emit_other_sections)
>        || (!emit_other_sections && !empty_debug_line))
>

I have run into this issue before.  "as -g" shouldn't silently
generate incorrect
debug info when input assembly codes already contain debug directives.
AS should either issue an error or ignore -g.  In either case, we need
a testcase
to verify it.

-- 
H.J.

Re: Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

[Mark Wielaard]

Hi,

On Wed, Aug 26, 2020 at 04:38:21PM -0700, H.J. Lu wrote:
> On Wed, Aug 26, 2020 at 2:38 PM Mark Wielaard <mark@klomp.org> wrote:
> > Would it be possible to have something like the following in gas, so
> > that it doesn't try generating a .debug_line section if there already
> > is one, even when -gdwarf-N is given (unless the assembly also
> > contains .loc directives because that shows the user is really
> > confused)?
> >
> > diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
> > index e4ba56d82ba..c0c09f4e9d0 100644
> > --- a/gas/dwarf2dbg.c
> > +++ b/gas/dwarf2dbg.c
> > @@ -2626,7 +2626,7 @@ dwarf2_init (void)
> >
> >
> >  /* Finish the dwarf2 debug sections.  We emit .debug.line if there
> > -   were any .file/.loc directives, or --gdwarf2 was given, or if the
> > +   were any .file/.loc directives, or --gdwarf2 was given, and if the
> >     file has a non-empty .debug_info section and an empty .debug_line
> >     section.  If we emit .debug_line, and the .debug_info section is
> >     empty, we also emit .debug_info, .debug_aranges and .debug_abbrev.
> > @@ -2650,9 +2650,16 @@ dwarf2_finish (void)
> >    empty_debug_line = line_seg == NULL || !seg_not_empty_p (line_seg);
> >
> >    /* We can't construct a new debug_line section if we already have one.
> > -     Give an error.  */
> > +     Give an error if we have seen any .loc, otherwise trust the user
> > +     knows what they are doing and want to generate the .debug_line
> > +     (and all other debug sections) themselves.  */
> >    if (all_segs && !empty_debug_line)
> > -    as_fatal ("duplicate .debug_line sections");
> > +    {
> > +      if (dwarf2_loc_directive_seen)
> > +       as_fatal ("duplicate .debug_line sections");
> > +      else
> > +       return;
> > +    }
> >
> >    if ((!all_segs && emit_other_sections)
> >        || (!emit_other_sections && !empty_debug_line))
> >
>
> I have run into this issue before.  "as -g" shouldn't silently
> generate incorrect debug info when input assembly codes already
> contain debug directives.  AS should either issue an error or
> ignore -g.

Right, that is what this patch does for .debug_line.  gas already
doesn't generate .debug_info, .debug_aranges and .debug_abbrev if
.debug_info is non-empty, even if -g is given.

> In either case, we need a testcase  to verify it.

Right, and the documentation needs to be update.  But first we have to
know whether the gas maintainers think this is the right approach.

Cheers,

Mark

Re: Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

[H.J. Lu]

On Sat, Aug 29, 2020 at 5:24 AM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi,
>
> On Wed, Aug 26, 2020 at 04:38:21PM -0700, H.J. Lu wrote:
> > On Wed, Aug 26, 2020 at 2:38 PM Mark Wielaard <mark@klomp.org> wrote:
> > > Would it be possible to have something like the following in gas, so
> > > that it doesn't try generating a .debug_line section if there already
> > > is one, even when -gdwarf-N is given (unless the assembly also
> > > contains .loc directives because that shows the user is really
> > > confused)?
> > >
> > > diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
> > > index e4ba56d82ba..c0c09f4e9d0 100644
> > > --- a/gas/dwarf2dbg.c
> > > +++ b/gas/dwarf2dbg.c
> > > @@ -2626,7 +2626,7 @@ dwarf2_init (void)
> > >
> > >
> > >  /* Finish the dwarf2 debug sections.  We emit .debug.line if there
> > > -   were any .file/.loc directives, or --gdwarf2 was given, or if the
> > > +   were any .file/.loc directives, or --gdwarf2 was given, and if the
> > >     file has a non-empty .debug_info section and an empty .debug_line
> > >     section.  If we emit .debug_line, and the .debug_info section is
> > >     empty, we also emit .debug_info, .debug_aranges and .debug_abbrev.
> > > @@ -2650,9 +2650,16 @@ dwarf2_finish (void)
> > >    empty_debug_line = line_seg == NULL || !seg_not_empty_p (line_seg);
> > >
> > >    /* We can't construct a new debug_line section if we already have one.
> > > -     Give an error.  */
> > > +     Give an error if we have seen any .loc, otherwise trust the user
> > > +     knows what they are doing and want to generate the .debug_line
> > > +     (and all other debug sections) themselves.  */
> > >    if (all_segs && !empty_debug_line)
> > > -    as_fatal ("duplicate .debug_line sections");
> > > +    {
> > > +      if (dwarf2_loc_directive_seen)
> > > +       as_fatal ("duplicate .debug_line sections");
> > > +      else
> > > +       return;
> > > +    }
> > >
> > >    if ((!all_segs && emit_other_sections)
> > >        || (!emit_other_sections && !empty_debug_line))
> > >
> >
> > I have run into this issue before.  "as -g" shouldn't silently
> > generate incorrect debug info when input assembly codes already
> > contain debug directives.  AS should either issue an error or
> > ignore -g.
>
> Right, that is what this patch does for .debug_line.  gas already
> doesn't generate .debug_info, .debug_aranges and .debug_abbrev if
> .debug_info is non-empty, even if -g is given.
>
> > In either case, we need a testcase  to verify it.
>
> Right, and the documentation needs to be update.  But first we have to
> know whether the gas maintainers think this is the right approach.
>

-g should be ignored in this case.


-- 
H.J.

Re: Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

[Mark Wielaard]

Hi,

On Sat, Aug 29, 2020 at 07:34:35AM -0700, H.J. Lu wrote:
> On Sat, Aug 29, 2020 at 5:24 AM Mark Wielaard <mark@klomp.org> wrote:
> > On Wed, Aug 26, 2020 at 04:38:21PM -0700, H.J. Lu wrote:
> > > On Wed, Aug 26, 2020 at 2:38 PM Mark Wielaard <mark@klomp.org> wrote:
> > > > Would it be possible to have something like the following in gas, so
> > > > that it doesn't try generating a .debug_line section if there already
> > > > is one, even when -gdwarf-N is given (unless the assembly also
> > > > contains .loc directives because that shows the user is really
> > > > confused)?
> > > >
> > > > diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
> > > > index e4ba56d82ba..c0c09f4e9d0 100644
> > > > --- a/gas/dwarf2dbg.c
> > > > +++ b/gas/dwarf2dbg.c
> > > > @@ -2626,7 +2626,7 @@ dwarf2_init (void)
> > > >
> > > >
> > > >  /* Finish the dwarf2 debug sections.  We emit .debug.line if there
> > > > -   were any .file/.loc directives, or --gdwarf2 was given, or if the
> > > > +   were any .file/.loc directives, or --gdwarf2 was given, and if the
> > > >     file has a non-empty .debug_info section and an empty .debug_line
> > > >     section.  If we emit .debug_line, and the .debug_info section is
> > > >     empty, we also emit .debug_info, .debug_aranges and .debug_abbrev.
> > > > @@ -2650,9 +2650,16 @@ dwarf2_finish (void)
> > > >    empty_debug_line = line_seg == NULL || !seg_not_empty_p (line_seg);
> > > >
> > > >    /* We can't construct a new debug_line section if we already have one.
> > > > -     Give an error.  */
> > > > +     Give an error if we have seen any .loc, otherwise trust the user
> > > > +     knows what they are doing and want to generate the .debug_line
> > > > +     (and all other debug sections) themselves.  */
> > > >    if (all_segs && !empty_debug_line)
> > > > -    as_fatal ("duplicate .debug_line sections");
> > > > +    {
> > > > +      if (dwarf2_loc_directive_seen)
> > > > +       as_fatal ("duplicate .debug_line sections");
> > > > +      else
> > > > +       return;
> > > > +    }
> > > >
> > > >    if ((!all_segs && emit_other_sections)
> > > >        || (!emit_other_sections && !empty_debug_line))
> > >
> > > I have run into this issue before.  "as -g" shouldn't silently
> > > generate incorrect debug info when input assembly codes already
> > > contain debug directives.  AS should either issue an error or
> > > ignore -g.
> >
> > Right, that is what this patch does for .debug_line.  gas already
> > doesn't generate .debug_info, .debug_aranges and .debug_abbrev if
> > .debug_info is non-empty, even if -g is given.
> >
> > > In either case, we need a testcase  to verify it.
> >
> > Right, and the documentation needs to be update.  But first we have to
> > know whether the gas maintainers think this is the right approach.
> 
> -g should be ignored in this case.

I am not sure what you mean by "in this case", or what precisely it
means to "ignore -g".

My proposal, and what my strawman patch implements, is that gas will
generate a .debug_line section when -g is given and the debug types is
DWARF (just as it does now). Unless there is a non-empty .debug_line
section already created by the input assembly and the input assembly
does not contain any .loc directive then gas will not try to generate
a .debug_line section itself but leaves the non-empty .debug_line as
is (currently gas will generate an error in this case). But if the
input assembly does contain both .loc directives and creates a
non-empty .debug line section gas will still generate an error (as it
does now, whether or not the input assembly contains any .loc
directives).

Does this sound sane?

Thanks,

Mark

Re: Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

[H.J. Lu]

On Sat, Aug 29, 2020 at 8:23 AM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi,
>
> On Sat, Aug 29, 2020 at 07:34:35AM -0700, H.J. Lu wrote:
> > On Sat, Aug 29, 2020 at 5:24 AM Mark Wielaard <mark@klomp.org> wrote:
> > > On Wed, Aug 26, 2020 at 04:38:21PM -0700, H.J. Lu wrote:
> > > > On Wed, Aug 26, 2020 at 2:38 PM Mark Wielaard <mark@klomp.org> wrote:
> > > > > Would it be possible to have something like the following in gas, so
> > > > > that it doesn't try generating a .debug_line section if there already
> > > > > is one, even when -gdwarf-N is given (unless the assembly also
> > > > > contains .loc directives because that shows the user is really
> > > > > confused)?
> > > > >
> > > > > diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
> > > > > index e4ba56d82ba..c0c09f4e9d0 100644
> > > > > --- a/gas/dwarf2dbg.c
> > > > > +++ b/gas/dwarf2dbg.c
> > > > > @@ -2626,7 +2626,7 @@ dwarf2_init (void)
> > > > >
> > > > >
> > > > >  /* Finish the dwarf2 debug sections.  We emit .debug.line if there
> > > > > -   were any .file/.loc directives, or --gdwarf2 was given, or if the
> > > > > +   were any .file/.loc directives, or --gdwarf2 was given, and if the
> > > > >     file has a non-empty .debug_info section and an empty .debug_line
> > > > >     section.  If we emit .debug_line, and the .debug_info section is
> > > > >     empty, we also emit .debug_info, .debug_aranges and .debug_abbrev.
> > > > > @@ -2650,9 +2650,16 @@ dwarf2_finish (void)
> > > > >    empty_debug_line = line_seg == NULL || !seg_not_empty_p (line_seg);
> > > > >
> > > > >    /* We can't construct a new debug_line section if we already have one.
> > > > > -     Give an error.  */
> > > > > +     Give an error if we have seen any .loc, otherwise trust the user
> > > > > +     knows what they are doing and want to generate the .debug_line
> > > > > +     (and all other debug sections) themselves.  */
> > > > >    if (all_segs && !empty_debug_line)
> > > > > -    as_fatal ("duplicate .debug_line sections");
> > > > > +    {
> > > > > +      if (dwarf2_loc_directive_seen)
> > > > > +       as_fatal ("duplicate .debug_line sections");
> > > > > +      else
> > > > > +       return;
> > > > > +    }
> > > > >
> > > > >    if ((!all_segs && emit_other_sections)
> > > > >        || (!emit_other_sections && !empty_debug_line))
> > > >
> > > > I have run into this issue before.  "as -g" shouldn't silently
> > > > generate incorrect debug info when input assembly codes already
> > > > contain debug directives.  AS should either issue an error or
> > > > ignore -g.
> > >
> > > Right, that is what this patch does for .debug_line.  gas already
> > > doesn't generate .debug_info, .debug_aranges and .debug_abbrev if
> > > .debug_info is non-empty, even if -g is given.
> > >
> > > > In either case, we need a testcase  to verify it.
> > >
> > > Right, and the documentation needs to be update.  But first we have to
> > > know whether the gas maintainers think this is the right approach.
> >
> > -g should be ignored in this case.
>
> I am not sure what you mean by "in this case", or what precisely it
> means to "ignore -g".
>
> My proposal, and what my strawman patch implements, is that gas will
> generate a .debug_line section when -g is given and the debug types is
> DWARF (just as it does now). Unless there is a non-empty .debug_line
> section already created by the input assembly and the input assembly
> does not contain any .loc directive then gas will not try to generate
> a .debug_line section itself but leaves the non-empty .debug_line as
> is (currently gas will generate an error in this case). But if the
> input assembly does contain both .loc directives and creates a
> non-empty .debug line section gas will still generate an error (as it
> does now, whether or not the input assembly contains any .loc
> directives).
>
> Does this sound sane?

What if there is a .file directive,  but without .loc directive, like

$ gcc -c x.c -Wa,-g


-- 
H.J.

Re: Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

[Mark Wielaard]

Hi,

On Sat, Aug 29, 2020 at 08:43:30AM -0700, H.J. Lu wrote:
> On Sat, Aug 29, 2020 at 8:23 AM Mark Wielaard <mark@klomp.org> wrote:
> > My proposal, and what my strawman patch implements, is that gas will
> > generate a .debug_line section when -g is given and the debug types is
> > DWARF (just as it does now). Unless there is a non-empty .debug_line
> > section already created by the input assembly and the input assembly
> > does not contain any .loc directive then gas will not try to generate
> > a .debug_line section itself but leaves the non-empty .debug_line as
> > is (currently gas will generate an error in this case). But if the
> > input assembly does contain both .loc directives and creates a
> > non-empty .debug line section gas will still generate an error (as it
> > does now, whether or not the input assembly contains any .loc
> > directives).
> >
> > Does this sound sane?
> 
> What if there is a .file directive,  but without .loc directive, like
> 
> $ gcc -c x.c -Wa,-g

That situation does not change, since in that case no .debug_*
sections are generated in the assembly file, so gas will generate
everything it currently generates.

Cheers,

Mark

Re: Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

[H.J. Lu]

On Sat, Aug 29, 2020 at 9:33 AM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi,
>
> On Sat, Aug 29, 2020 at 08:43:30AM -0700, H.J. Lu wrote:
> > On Sat, Aug 29, 2020 at 8:23 AM Mark Wielaard <mark@klomp.org> wrote:
> > > My proposal, and what my strawman patch implements, is that gas will
> > > generate a .debug_line section when -g is given and the debug types is
> > > DWARF (just as it does now). Unless there is a non-empty .debug_line
> > > section already created by the input assembly and the input assembly
> > > does not contain any .loc directive then gas will not try to generate
> > > a .debug_line section itself but leaves the non-empty .debug_line as
> > > is (currently gas will generate an error in this case). But if the
> > > input assembly does contain both .loc directives and creates a
> > > non-empty .debug line section gas will still generate an error (as it
> > > does now, whether or not the input assembly contains any .loc
> > > directives).
> > >
> > > Does this sound sane?
> >
> > What if there is a .file directive,  but without .loc directive, like
> >
> > $ gcc -c x.c -Wa,-g
>
> That situation does not change, since in that case no .debug_*
> sections are generated in the assembly file, so gas will generate
> everything it currently generates.
>

Will line info be correct in this case?


-- 
H.J.

Re: Duplicate .debug_lines (Was: [PATCH 5/5] Add --gdwarf-5 to ASM_SPEC)

[Mark Wielaard]

H.J. Lu wrote:
> On Sat, Aug 29, 2020 at 9:33 AM Mark Wielaard <mark@klomp.org> wrote:
>> Hi,
>>
>> On Sat, Aug 29, 2020 at 08:43:30AM -0700, H.J. Lu wrote:
>>> On Sat, Aug 29, 2020 at 8:23 AM Mark Wielaard <mark@klomp.org> wrote:
>>>> My proposal, and what my strawman patch implements, is that gas will
>>>> generate a .debug_line section when -g is given and the debug types is
>>>> DWARF (just as it does now). Unless there is a non-empty .debug_line
>>>> section already created by the input assembly and the input assembly
>>>> does not contain any .loc directive then gas will not try to generate
>>>> a .debug_line section itself but leaves the non-empty .debug_line as
>>>> is (currently gas will generate an error in this case). But if the
>>>> input assembly does contain both .loc directives and creates a
>>>> non-empty .debug line section gas will still generate an error (as it
>>>> does now, whether or not the input assembly contains any .loc
>>>> directives).
>>>>
>>>> Does this sound sane?
>>>
>>> What if there is a .file directive,  but without .loc directive, like
>>>
>>> $ gcc -c x.c -Wa,-g
>>
>> That situation does not change, since in that case no .debug_*
>> sections are generated in the assembly file, so gas will generate
>> everything it currently generates.
>
> Will line info be correct in this case?

Nothing about how gas generates the line table will change with the above proposal. It only changes the behaviour when the assembly already contains a line table.

[PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Mark Wielaard]

When -g was used to generate DWARF gas would error out when a .debug_line
already exists. But when a .debug_info section already exists it would
simply skip generating one without warning or error. Do the same for
.debug_line. It is only an error when the user explicitly uses .loc
directives and also generates the .debug_line table itself.

The tests are unfortunately arch specific because the line table is only
generated when actual instructions have been emitted. Use i386 because
that is probably the most used architecture. Before this patch the new
dwarf-line-2 testcase would fail, with this patch it succeeds (and doesn't
try to add its own line table).

gas/ChangeLog:

    * as.texi (-g): Explicitly mention when .debug_info and .debug_line
    are generated for the DWARF format.
    (Loc): Add that it is an error to both use a .loc directive and
    generate a .debug_line yourself.
    * dwarf2dbg.c (dwarf2_any_loc_directive_seen): New static variable.
    (dwarf2_directive_loc): Set dwarf2_any_loc_directive_seen to TRUE.
    (dwarf2_finish): Check dwarf2_any_loc_directive_seen before emitting
    an error. Only create .debug_line if it is empty (or doesn't exist).
    * testsuite/gas/i386/i386.exp: Add dwarf2-line-{1,2,3,4} when testing
    an elf target.
    * testsuite/gas/i386/dwarf2-line-{1,2,3,4}.{s,d,l}: New test files.
---
 gas/ChangeLog                          | 14 ++++
 gas/doc/as.texi                        |  7 +-
 gas/dwarf2dbg.c                        | 29 +++++---
 gas/testsuite/gas/i386/dwarf2-line-1.d | 45 +++++++++++++
 gas/testsuite/gas/i386/dwarf2-line-1.s | 28 ++++++++
 gas/testsuite/gas/i386/dwarf2-line-2.d | 48 ++++++++++++++
 gas/testsuite/gas/i386/dwarf2-line-2.s | 91 ++++++++++++++++++++++++++
 gas/testsuite/gas/i386/dwarf2-line-3.d |  3 +
 gas/testsuite/gas/i386/dwarf2-line-3.l |  2 +
 gas/testsuite/gas/i386/dwarf2-line-3.s | 32 +++++++++
 gas/testsuite/gas/i386/dwarf2-line-4.d | 46 +++++++++++++
 gas/testsuite/gas/i386/dwarf2-line-4.s | 29 ++++++++
 gas/testsuite/gas/i386/i386.exp        |  5 ++
 13 files changed, 369 insertions(+), 10 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/dwarf2-line-1.d
 create mode 100644 gas/testsuite/gas/i386/dwarf2-line-1.s
 create mode 100644 gas/testsuite/gas/i386/dwarf2-line-2.d
 create mode 100644 gas/testsuite/gas/i386/dwarf2-line-2.s
 create mode 100644 gas/testsuite/gas/i386/dwarf2-line-3.d
 create mode 100644 gas/testsuite/gas/i386/dwarf2-line-3.l
 create mode 100644 gas/testsuite/gas/i386/dwarf2-line-3.s
 create mode 100644 gas/testsuite/gas/i386/dwarf2-line-4.d
 create mode 100644 gas/testsuite/gas/i386/dwarf2-line-4.s

diff --git a/gas/ChangeLog b/gas/ChangeLog
index d34c08e924c..70d09729443 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,17 @@
+2020-09-07  Mark Wielaard  <mark@klomp.org>
+
+	* as.texi (-g): Explicitly mention when .debug_info and .debug_line
+	are generated for the DWARF format.
+	(Loc): Add that it is an error to both use a .loc directive and
+	generate a .debug_line yourself.
+	* dwarf2dbg.c (dwarf2_any_loc_directive_seen): New static variable.
+	(dwarf2_directive_loc): Set dwarf2_any_loc_directive_seen to TRUE.
+	(dwarf2_finish): Check dwarf2_any_loc_directive_seen before emitting
+	an error. Only create .debug_line if it is empty (or doesn't exist).
+	* testsuite/gas/i386/i386.exp: Add dwarf2-line-{1,2,3,4} when testing
+	an elf target.
+	* testsuite/gas/i386/dwarf2-line-{1,2,3,4}.{s,d,l}: New test files.
+
 2020-09-04  Mark Wielaard  <mark@klomp.org>
 
 	* dwarf2dbg.c (add_line_strp): New function.
diff --git a/gas/doc/as.texi b/gas/doc/as.texi
index 112eaf810cd..f2a0314310d 100644
--- a/gas/doc/as.texi
+++ b/gas/doc/as.texi
@@ -745,7 +745,9 @@ compiler output).
 @itemx --gen-debug
 Generate debugging information for each assembler source line using whichever
 debug format is preferred by the target.  This currently means either STABS,
-ECOFF or DWARF2.
+ECOFF or DWARF2.  When the debug format is DWARF then a @code{.debug_info} and
+@code{.debug_line} section is only emitted when the assembly file doesn't
+generate one itself.
 
 @item --gstabs
 Generate stabs debugging information for each assembler line.  This
@@ -5857,7 +5859,8 @@ the @code{.loc} directive will add a row to the @code{.debug_line} line
 number matrix corresponding to the immediately following assembly
 instruction.  The @var{fileno}, @var{lineno}, and optional @var{column}
 arguments will be applied to the @code{.debug_line} state machine before
-the row is added.
+the row is added.  It is an error for the input assembly file to generate
+a non-empty @code{.debug_line} and also use @code{loc} directives.
 
 The @var{options} are a sequence of the following tokens in any order:
 
diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
index e0ea3991b45..1c21d58c591 100644
--- a/gas/dwarf2dbg.c
+++ b/gas/dwarf2dbg.c
@@ -226,9 +226,15 @@ static unsigned int  dirs_in_use = 0;
 static unsigned int  dirs_allocated = 0;
 
 /* TRUE when we've seen a .loc directive recently.  Used to avoid
-   doing work when there's nothing to do.  */
+   doing work when there's nothing to do.  Will be reset by
+   dwarf2_consume_line_info.  */
 bfd_boolean dwarf2_loc_directive_seen;
 
+/* TRUE when we've seen any .loc directive at any time during parsing.
+   Indicates the user wants us to generate a .debug_line section.
+   Used in dwarf2_finish as sanity check.  */
+static bfd_boolean dwarf2_any_loc_directive_seen;
+
 /* TRUE when we're supposed to set the basic block mark whenever a
    label is seen.  */
 bfd_boolean dwarf2_loc_mark_labels;
@@ -1290,7 +1296,7 @@ dwarf2_directive_loc (int dummy ATTRIBUTE_UNUSED)
     }
 
   demand_empty_rest_of_line ();
-  dwarf2_loc_directive_seen = TRUE;
+  dwarf2_any_loc_directive_seen = dwarf2_loc_directive_seen = TRUE;
   debug_type = DEBUG_NONE;
 
   /* If we were given a view id, emit the row right away.  */
@@ -2737,7 +2743,7 @@ dwarf2_init (void)
 
 
 /* Finish the dwarf2 debug sections.  We emit .debug.line if there
-   were any .file/.loc directives, or --gdwarf2 was given, or if the
+   were any .file/.loc directives, or --gdwarf2 was given, and if the
    file has a non-empty .debug_info section and an empty .debug_line
    section.  If we emit .debug_line, and the .debug_info section is
    empty, we also emit .debug_info, .debug_aranges and .debug_abbrev.
@@ -2761,8 +2767,10 @@ dwarf2_finish (void)
   empty_debug_line = line_seg == NULL || !seg_not_empty_p (line_seg);
 
   /* We can't construct a new debug_line section if we already have one.
-     Give an error.  */
-  if (all_segs && !empty_debug_line)
+     Give an error if we have seen any .loc, otherwise trust the user
+     knows what they are doing and want to generate the .debug_line
+     (and all other debug sections) themselves.  */
+  if (all_segs && !empty_debug_line && dwarf2_any_loc_directive_seen)
     as_fatal ("duplicate .debug_line sections");
 
   if ((!all_segs && emit_other_sections)
@@ -2776,8 +2784,12 @@ dwarf2_finish (void)
   sizeof_address = DWARF2_ADDR_SIZE (stdoutput);
 
   /* Create and switch to the line number section.  */
-  line_seg = subseg_new (".debug_line", 0);
-  bfd_set_section_flags (line_seg, SEC_READONLY | SEC_DEBUGGING | SEC_OCTETS);
+  if (empty_debug_line)
+    {
+      line_seg = subseg_new (".debug_line", 0);
+      bfd_set_section_flags (line_seg,
+			     SEC_READONLY | SEC_DEBUGGING | SEC_OCTETS);
+    }
 
   /* For each subsection, chain the debug entries together.  */
   for (s = all_segs; s; s = s->next)
@@ -2803,7 +2815,8 @@ dwarf2_finish (void)
 	}
     }
 
-  out_debug_line (line_seg);
+  if (empty_debug_line)
+    out_debug_line (line_seg);
 
   /* If this is assembler generated line info, and there is no
      debug_info already, we need .debug_info, .debug_abbrev and
diff --git a/gas/testsuite/gas/i386/dwarf2-line-1.d b/gas/testsuite/gas/i386/dwarf2-line-1.d
new file mode 100644
index 00000000000..196f099c1a8
--- /dev/null
+++ b/gas/testsuite/gas/i386/dwarf2-line-1.d
@@ -0,0 +1,45 @@
+#as: -gdwarf-2
+#readelf: -wl
+#name: DWARF .debug_line 1
+
+Raw dump of debug contents of section \.z?debug_line:
+
+  Offset:                      0x0
+  Length:                      .*
+  DWARF Version:               3
+  Prologue Length:             .*
+  Minimum Instruction Length:  1
+  Initial value of 'is_stmt':  1
+  Line Base:                   -5
+  Line Range:                  14
+  Opcode Base:                 13
+
+ Opcodes:
+  Opcode 1 has 0 args
+  Opcode 2 has 1 arg
+  Opcode 3 has 1 arg
+  Opcode 4 has 1 arg
+  Opcode 5 has 1 arg
+  Opcode 6 has 0 args
+  Opcode 7 has 0 args
+  Opcode 8 has 0 args
+  Opcode 9 has 1 arg
+  Opcode 10 has 0 args
+  Opcode 11 has 0 args
+  Opcode 12 has 1 arg
+
+ The Directory Table \(offset 0x.*\):
+  .*
+
+ The File Name Table \(offset 0x.*\):
+  Entry	Dir	Time	Size	Name
+  1	1	0	0	dwarf2-line-1.s
+
+ Line Number Statements:
+  \[0x.*\]  Extended opcode 2: set Address to 0x0
+  \[0x.*\]  Special opcode 13: advance Address by 0 to 0x0 and Line by 8 to 9
+  \[0x.*\]  Special opcode 20: advance Address by 1 to 0x1 and Line by 1 to 10
+  \[0x.*\]  Advance PC by 1 to 0x2
+  \[0x.*\]  Extended opcode 1: End of Sequence
+
+
diff --git a/gas/testsuite/gas/i386/dwarf2-line-1.s b/gas/testsuite/gas/i386/dwarf2-line-1.s
new file mode 100644
index 00000000000..a2cb22e842a
--- /dev/null
+++ b/gas/testsuite/gas/i386/dwarf2-line-1.s
@@ -0,0 +1,28 @@
+        .file   "dwarf2-test.c"
+        .text
+        .section .text.startup,"ax",@progbits
+        .p2align 4
+        .globl  main
+        .type   main, @function
+main:
+        .cfi_startproc
+        nop
+        ret
+        .cfi_endproc
+        .size   main, .-main
+        .text
+
+        .section .debug_info,"",%progbits
+        .long   0x0
+        .value  0x2
+        .long   .Ldebug_abbrev0
+        .byte   0x8
+        .uleb128 0x1
+
+        .section .debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+        .uleb128 0x0    # (abbrev code)
+        .uleb128 0x0    # (abbrev code)
+        .uleb128 0x0    # (abbrev code)
+
+# No .debug_line ok even if there is a .debug_info section
diff --git a/gas/testsuite/gas/i386/dwarf2-line-2.d b/gas/testsuite/gas/i386/dwarf2-line-2.d
new file mode 100644
index 00000000000..2553fea32cd
--- /dev/null
+++ b/gas/testsuite/gas/i386/dwarf2-line-2.d
@@ -0,0 +1,48 @@
+#as: -gdwarf-2
+#readelf: -wl
+#name: DWARF .debug_line 2
+
+Raw dump of debug contents of section .z?debug_line:
+
+  Offset:                      0x0
+  Length:                      62
+  DWARF Version:               .
+  Prologue Length:             35
+  Minimum Instruction Length:  1
+  Initial value of 'is_stmt':  1
+  Line Base:                   1
+  Line Range:                  1
+  Opcode Base:                 16
+
+ Opcodes:
+  Opcode 1 has 0 args
+  Opcode 2 has 1 arg
+  Opcode 3 has 1 arg
+  Opcode 4 has 1 arg
+  Opcode 5 has 1 arg
+  Opcode 6 has 0 args
+  Opcode 7 has 0 args
+  Opcode 8 has 0 args
+  Opcode 9 has 1 arg
+  Opcode 10 has 0 args
+  Opcode 11 has 0 args
+  Opcode 12 has 1 arg
+  Opcode 13 has 0 args
+  Opcode 14 has 0 args
+  Opcode 15 has 0 args
+
+ The Directory Table is empty.
+
+ The File Name Table \(offset 0x.*\):
+  Entry	Dir	Time	Size	Name
+  1	0	0	0	file1.txt
+
+ Line Number Statements:
+  \[0x.*\]  Extended opcode 2: set Address to 0x0
+  \[0x.*\]  Advance Line by 3 to 4
+  \[0x.*\]  Copy
+  \[0x.*\]  Copy \(view 1\)
+  \[0x.*\]  Extended opcode 2: set Address to 0x0
+  \[0x.*\]  Extended opcode 1: End of Sequence
+
+
diff --git a/gas/testsuite/gas/i386/dwarf2-line-2.s b/gas/testsuite/gas/i386/dwarf2-line-2.s
new file mode 100644
index 00000000000..63dedeb6594
--- /dev/null
+++ b/gas/testsuite/gas/i386/dwarf2-line-2.s
@@ -0,0 +1,91 @@
+        .file   "dwarf2-test.c"
+        .text
+        .section .text.startup,"ax",@progbits
+        .p2align 4
+        .globl  main
+        .type   main, @function
+main:
+        .cfi_startproc
+        nop
+        ret
+        .cfi_endproc
+        .size   main, .-main
+        .text
+
+        .section .debug_info,"",%progbits
+        .long   0x0
+        .value  0x2
+        .long   .Ldebug_abbrev0
+        .byte   0x8
+        .uleb128 0x1
+
+        .section .debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+        .uleb128 0x0    # (abbrev code)
+        .uleb128 0x0    # (abbrev code)
+        .uleb128 0x0    # (abbrev code)
+
+# A non-empty .debug_line section is ok when not using .loc directives
+	.section .debug_line
+.Lline1_begin:
+	.4byte		.Lline1_end - .Lline1_start	/* Initial length */
+.Lline1_start:
+	.2byte		2			/* Version */
+	.4byte		.Lline1_lines - .Lline1_hdr	/* header_length */
+.Lline1_hdr:
+	.byte		1			/* Minimum insn length */
+	.byte		1			/* default_is_stmt */
+	.byte		1			/* line_base */
+ 	.byte		1			/* line_range */
+	.byte		0x10			/* opcode_base */
+
+	/* Standard lengths */
+	.byte		0
+	.byte		1
+	.byte		1
+	.byte		1
+	.byte		1
+	.byte		0
+	.byte		0
+	.byte		0
+	.byte		1
+	.byte		0
+	.byte		0
+	.byte		1
+	.byte		0
+	.byte		0
+	.byte		0
+
+	/* Include directories */
+	.byte		0
+
+	/* File names */
+	.ascii		"file1.txt\0"
+	.uleb128	0
+	.uleb128	0
+	.uleb128	0
+
+	.byte		0
+
+.Lline1_lines:
+	.byte		0	/* DW_LNE_set_address */
+	.uleb128	5
+	.byte		2
+	.4byte		.Lbegin_func_cu1
+
+	.byte		3	/* DW_LNS_advance_line */
+	.sleb128	3	/* ... to 4 */
+
+	.byte		1	/* DW_LNS_copy */
+
+	.byte		1	/* DW_LNS_copy (second time as an end-of-prologue marker) */
+
+	.byte		0	/* DW_LNE_set_address */
+	.uleb128	5
+	.byte		2
+	.4byte		.Lend_func_cu1
+
+	.byte		0	/* DW_LNE_end_of_sequence */
+	.uleb128	1
+	.byte		1
+
diff --git a/gas/testsuite/gas/i386/dwarf2-line-3.d b/gas/testsuite/gas/i386/dwarf2-line-3.d
new file mode 100644
index 00000000000..42c94c56821
--- /dev/null
+++ b/gas/testsuite/gas/i386/dwarf2-line-3.d
@@ -0,0 +1,3 @@
+#as: -gdwarf-2
+#name: DWARF .debug_line 3
+#error_output: dwarf2-line-3.l
diff --git a/gas/testsuite/gas/i386/dwarf2-line-3.l b/gas/testsuite/gas/i386/dwarf2-line-3.l
new file mode 100644
index 00000000000..c26e1fa8782
--- /dev/null
+++ b/gas/testsuite/gas/i386/dwarf2-line-3.l
@@ -0,0 +1,2 @@
+[^:]*: Assembler messages:
+[^:]*: Fatal error: duplicate .debug_line sections
diff --git a/gas/testsuite/gas/i386/dwarf2-line-3.s b/gas/testsuite/gas/i386/dwarf2-line-3.s
new file mode 100644
index 00000000000..2085ef93940
--- /dev/null
+++ b/gas/testsuite/gas/i386/dwarf2-line-3.s
@@ -0,0 +1,32 @@
+        .file   "dwarf2-test.c"
+        .text
+        .section .text.startup,"ax",@progbits
+        .p2align 4
+        .globl  main
+        .type   main, @function
+main:
+        .cfi_startproc
+        nop
+	.loc 1 1
+        ret
+        .cfi_endproc
+        .size   main, .-main
+        .text
+
+        .section .debug_info,"",%progbits
+        .long   0x0
+        .value  0x2
+        .long   .Ldebug_abbrev0
+        .byte   0x8
+        .uleb128 0x1
+
+        .section .debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+        .uleb128 0x0    # (abbrev code)
+        .uleb128 0x0    # (abbrev code)
+        .uleb128 0x0    # (abbrev code)
+
+# A non-empty .debug_line section is NOT ok when using .loc directives
+        .section .debug_line,"",@progbits
+.Ldebug_line0:
+        .byte 0x1
diff --git a/gas/testsuite/gas/i386/dwarf2-line-4.d b/gas/testsuite/gas/i386/dwarf2-line-4.d
new file mode 100644
index 00000000000..c0c85f4639f
--- /dev/null
+++ b/gas/testsuite/gas/i386/dwarf2-line-4.d
@@ -0,0 +1,46 @@
+#as: -gdwarf-2
+#readelf: -wl
+#name: DWARF .debug_line 4
+
+Raw dump of debug contents of section \.z?debug_line:
+
+  Offset:                      0x0
+  Length:                      .*
+  DWARF Version:               3
+  Prologue Length:             .*
+  Minimum Instruction Length:  1
+  Initial value of 'is_stmt':  1
+  Line Base:                   -5
+  Line Range:                  14
+  Opcode Base:                 13
+
+ Opcodes:
+  Opcode 1 has 0 args
+  Opcode 2 has 1 arg
+  Opcode 3 has 1 arg
+  Opcode 4 has 1 arg
+  Opcode 5 has 1 arg
+  Opcode 6 has 0 args
+  Opcode 7 has 0 args
+  Opcode 8 has 0 args
+  Opcode 9 has 1 arg
+  Opcode 10 has 0 args
+  Opcode 11 has 0 args
+  Opcode 12 has 1 arg
+
+ The Directory Table \(offset 0x.*\):
+  .*
+
+ The File Name Table \(offset 0x.*\):
+  Entry	Dir	Time	Size	Name
+  1	1	0	0	dwarf2-line-4.s
+
+ Line Number Statements:
+  \[0x.*\]  Extended opcode 2: set Address to 0x0
+  \[0x.*\]  Special opcode 13: advance Address by 0 to 0x0 and Line by 8 to 9
+  \[0x.*\]  Advance Line by -8 to 1
+  \[0x.*\]  Special opcode 19: advance Address by 1 to 0x1 and Line by 0 to 1
+  \[0x.*\]  Advance PC by 1 to 0x2
+  \[0x.*\]  Extended opcode 1: End of Sequence
+
+
diff --git a/gas/testsuite/gas/i386/dwarf2-line-4.s b/gas/testsuite/gas/i386/dwarf2-line-4.s
new file mode 100644
index 00000000000..89bb62d9db7
--- /dev/null
+++ b/gas/testsuite/gas/i386/dwarf2-line-4.s
@@ -0,0 +1,29 @@
+        .file   "dwarf2-test.c"
+        .text
+        .section .text.startup,"ax",@progbits
+        .p2align 4
+        .globl  main
+        .type   main, @function
+main:
+        .cfi_startproc
+        nop
+	.loc 1 1
+        ret
+        .cfi_endproc
+        .size   main, .-main
+        .text
+
+        .section .debug_info,"",%progbits
+        .long   0x0
+        .value  0x2
+        .long   .Ldebug_abbrev0
+        .byte   0x8
+        .uleb128 0x1
+
+        .section .debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+        .uleb128 0x0    # (abbrev code)
+        .uleb128 0x0    # (abbrev code)
+        .uleb128 0x0    # (abbrev code)
+
+# No .debug_line ok even if there is a .debug_info section and using .locs
diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
index 0a863ff3bd4..0efcc8c7c27 100644
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -599,6 +599,11 @@ if [expr ([istarget "i*86-*-*"] ||  [istarget "x86_64-*-*"]) && [gas_32_check]]
 	run_dump_test "localpic"
 	run_dump_test "debug1"
 
+	run_dump_test "dwarf2-line-1"
+	run_dump_test "dwarf2-line-2"
+	run_dump_test "dwarf2-line-3"
+	run_dump_test "dwarf2-line-4"
+
 	run_dump_test "dw2-compress-2"
 	run_dump_test "dw2-compressed-2"
 
-- 
2.18.4

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Nick Clifton]

Hi Mark,

> The tests are unfortunately arch specific because the line table is only
> generated when actual instructions have been emitted.

Are you sure about that ?  How about using the NOP instruction ?

I admit that some architectures do need special syntax for their
nop instructions, but there is already a test in gas testsuite
that handles this (see gas/testsuite/gas/all/gas.exp:org-1).  The
nop-selecting code there could probably be extracted into a library
function then used in lots of places...

Cheers
  Nick

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[H.J. Lu]

On Fri, Sep 11, 2020 at 5:48 AM Nick Clifton via Binutils
<binutils@sourceware.org> wrote:
>
> Hi Mark,
>
> > The tests are unfortunately arch specific because the line table is only
> > generated when actual instructions have been emitted.
>
> Are you sure about that ?  How about using the NOP instruction ?
>
> I admit that some architectures do need special syntax for their
> nop instructions, but there is already a test in gas testsuite
> that handles this (see gas/testsuite/gas/all/gas.exp:org-1).  The
> nop-selecting code there could probably be extracted into a library
> function then used in lots of places...
>

7.66 '.nops SIZE[, CONTROL]'
============================

This directive emits SIZE bytes filled with no-op instructions.  SIZE is
absolute expression, which must be a positve value.  CONTROL controls
how no-op instructions should be generated.  If the comma and CONTROL
are omitted, CONTROL is assumed to be zero.


-- 
H.J.

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Nick Clifton]

Hi H.J.

> 7.66 '.nops SIZE[, CONTROL]'

Thanks H.J.  I had completely forgotten that this has already been implemented.

Cheers
  Nick

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Mark Wielaard]

On Fri, 2020-09-11 at 06:51 -0700, H.J. Lu wrote:
> On Fri, Sep 11, 2020 at 5:48 AM Nick Clifton via Binutils
> <binutils@sourceware.org> wrote:
> > > The tests are unfortunately arch specific because the line table
> > > is only
> > > generated when actual instructions have been emitted.
> > 
> > Are you sure about that ?  How about using the NOP instruction ?
> > 
> > I admit that some architectures do need special syntax for their
> > nop instructions, but there is already a test in gas testsuite
> > that handles this (see gas/testsuite/gas/all/gas.exp:org-1).  The
> > nop-selecting code there could probably be extracted into a library
> > function then used in lots of places...
> 
> 7.66 '.nops SIZE[, CONTROL]'
> ============================
> 
> This directive emits SIZE bytes filled with no-op instructions.  SIZE
> is
> absolute expression, which must be a positve value.  CONTROL controls
> how no-op instructions should be generated.  If the comma and CONTROL
> are omitted, CONTROL is assumed to be zero.

O nice. Somewhat embarrassingly I hadn't seen that because I was using
the online documentation: 
https://sourceware.org/binutils/docs/as/Pseudo-Ops.html

Which I now realize is not current. Oops. Can someone (I?) update that?

Sadly .nops does produce one or more architecture specific nop
instructions, but does not result in a call to dwarf2_emit_insn like
real instructions. So it looks like I still need to use the
gas.exp:org-1 trick.

Or is it a bug that .nops doesn't trigger a call to dwarf2_imit_insn?

Another issue is that at least NEWS claims:

* Add support for .nops directive.  It is currently supported only for
  x86 targets.

Is that still true? If it isn't supported generically, it isn't really
that useful.

Thanks,

Mark

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Nick Clifton]

Hi Mark,

> O nice. Somewhat embarrassingly I hadn't seen that because I was using
> the online documentation: 
> https://sourceware.org/binutils/docs/as/Pseudo-Ops.html
> 
> Which I now realize is not current. Oops. Can someone (I?) update that?

Doh!  This was my fault.  When I updated the docs for the 2.35 release
I changed the symbol link for the docs/ directory to point to docs-2.25
instead of docs-2.35.  Silly me.  I have now fixed this...

Cheers
  Nick

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Nick Clifton]

Hi Mark,

> * Add support for .nops directive.  It is currently supported only for
>   x86 targets.
> 
> Is that still true? 

It is.  I have however just checked in a patch to add ".nop" directive
which does work for all architectures, and which does count as a valid
instruction for the purposes of DWARD line number table generation.

Cheers
  Nick

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Nick Clifton]

Hi Mark,

> gas/ChangeLog:
> 
>     * as.texi (-g): Explicitly mention when .debug_info and .debug_line
>     are generated for the DWARF format.
>     (Loc): Add that it is an error to both use a .loc directive and
>     generate a .debug_line yourself.
>     * dwarf2dbg.c (dwarf2_any_loc_directive_seen): New static variable.
>     (dwarf2_directive_loc): Set dwarf2_any_loc_directive_seen to TRUE.
>     (dwarf2_finish): Check dwarf2_any_loc_directive_seen before emitting
>     an error. Only create .debug_line if it is empty (or doesn't exist).
>     * testsuite/gas/i386/i386.exp: Add dwarf2-line-{1,2,3,4} when testing
>     an elf target.
>     * testsuite/gas/i386/dwarf2-line-{1,2,3,4}.{s,d,l}: New test files.

Approved - please apply.

If you are feeling keen, then do please have a go at creating a generic
testcase.  If not, then I will make one myself soon.

Cheers
  Nick

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Mark Wielaard]

Hi Nick,

On Fri, 2020-09-11 at 16:45 +0100, Nick Clifton wrote:
> > gas/ChangeLog:
> > 
> >     * as.texi (-g): Explicitly mention when .debug_info and .debug_line
> >     are generated for the DWARF format.
> >     (Loc): Add that it is an error to both use a .loc directive and
> >     generate a .debug_line yourself.
> >     * dwarf2dbg.c (dwarf2_any_loc_directive_seen): New static variable.
> >     (dwarf2_directive_loc): Set dwarf2_any_loc_directive_seen to TRUE.
> >     (dwarf2_finish): Check dwarf2_any_loc_directive_seen before emitting
> >     an error. Only create .debug_line if it is empty (or doesn't exist).
> >     * testsuite/gas/i386/i386.exp: Add dwarf2-line-{1,2,3,4} when testing
> >     an elf target.
> >     * testsuite/gas/i386/dwarf2-line-{1,2,3,4}.{s,d,l}: New test files.
> 
> Approved - please apply.

Thanks, pushed as commit edc7a80.

> If you are feeling keen, then do please have a go at creating a
> generic testcase.  If not, then I will make one myself soon.

Jakub was already working on one to be used as configure test for gcc:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553586.html

Once we have something that works I'll make sure to submit something
similar to the gas testsuite to make sure things keep working.

Which brings me to another request. This patch (and some of the other
dwarf5 fixes) as pre-requirements for passing --gdwarf-5 to gas from
gcc to get full DWARF5 support. Would it be possible to backport these
to the 2.35 branch so that it can be used with bintuils 2.35.1?

commit 19d80e5fec548e681c453d15b4ae5b49bc080acc
bfd: Handle DW_FORM_data16 for .debug_line tables containing MD5

commit c3757b583d2448a5996e83e374fb96ac7938da35
Fix the linker's handling of DWARF-5 line number tables.

commit 4a5700b62f767ed08c97122bad182244700bb4e3
gas: Handle bad -gdwarf options, just like bad --gdwarf options.

commit dd216e07a156c19e589fc5f481fe13a256262801
gas: Use DW_FORM_sec_offset for DWARF version 4 or higher.

commit b0b3ea7e10443f211267babcac99ac6677d593ce
gas: Make sure to only add an md5 to a .file when requested.

commit bdd3b953e24c3d2bc55e06e30e0ef5e0ff829e3f
gas: Output .debug_rnglists for DWARF 5.

commit d2a54558074287721ce9e47142f7fa92bda15531
gas: Output directory and file names in .debug_line_str for DWARF5

commit edc7a80a9c04667dfa74f90f74bd39827bc372b7
gas: Don't error when .debug_line already exists, unless .loc was used

I can do the backports myself if you tell me how.

Thanks,

Mark

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Nick Clifton]

Hi Mark,

> Which brings me to another request. This patch (and some of the other
> dwarf5 fixes) as pre-requirements for passing --gdwarf-5 to gas from
> gcc to get full DWARF5 support. Would it be possible to backport these
> to the 2.35 branch so that it can be used with bintuils 2.35.1?
> 
> commit 19d80e5fec548e681c453d15b4ae5b49bc080acc
> bfd: Handle DW_FORM_data16 for .debug_line tables containing MD5
> 
> commit c3757b583d2448a5996e83e374fb96ac7938da35
> Fix the linker's handling of DWARF-5 line number tables.
> 
> commit 4a5700b62f767ed08c97122bad182244700bb4e3
> gas: Handle bad -gdwarf options, just like bad --gdwarf options.
> 
> commit dd216e07a156c19e589fc5f481fe13a256262801
> gas: Use DW_FORM_sec_offset for DWARF version 4 or higher.
> 
> commit b0b3ea7e10443f211267babcac99ac6677d593ce
> gas: Make sure to only add an md5 to a .file when requested.
> 
> commit bdd3b953e24c3d2bc55e06e30e0ef5e0ff829e3f
> gas: Output .debug_rnglists for DWARF 5.
> 
> commit d2a54558074287721ce9e47142f7fa92bda15531
> gas: Output directory and file names in .debug_line_str for DWARF5
> 
> commit edc7a80a9c04667dfa74f90f74bd39827bc372b7
> gas: Don't error when .debug_line already exists, unless .loc was used
> 
> I can do the backports myself if you tell me how.

The how is quite straightforward.  You probably know most of this already
but I am going to include it here just in case:

  0. Request permission to backport the patches.  
     (Permission is granted for the ones listed above).
  1. Checkout the mainline binutils sources from the git repository.
  2. Get the patches for the commits above.
  3. Switch to the binutils-2_35 branch.
  4. Apply the patches.  
     The Changelog changes will probably not apply cleanly, but this is
     OK, because the convention is to create a new changelog entry with
     today's date saying something along the lines of "backport this patch
     from the mainline sources" and then including the original changelog
     entry as a comment in the new changelog.
  5. Test that the patched sources still build.
  6. Commit the changes.

Once this is complete it would be worth sending an email to the list suggesting
that the creation of a 2.35.1 release might be a good thing.  (Assuming of course
that you do think that it would be a good thing).

Cheers
  Nick

Re: [PATCH] gas: Don't error when .debug_line already exists, unless .loc was used

[Mark Wielaard]

Hi Nick,

On Mon, Sep 14, 2020 at 10:45:45AM +0100, Nick Clifton wrote:
> > Which brings me to another request. This patch (and some of the other
> > dwarf5 fixes) as pre-requirements for passing --gdwarf-5 to gas from
> > gcc to get full DWARF5 support. Would it be possible to backport these
> > to the 2.35 branch so that it can be used with bintuils 2.35.1?
> > 
> > commit 19d80e5fec548e681c453d15b4ae5b49bc080acc
> > bfd: Handle DW_FORM_data16 for .debug_line tables containing MD5
> > 
> > commit c3757b583d2448a5996e83e374fb96ac7938da35
> > Fix the linker's handling of DWARF-5 line number tables.
> > 
> > commit 4a5700b62f767ed08c97122bad182244700bb4e3
> > gas: Handle bad -gdwarf options, just like bad --gdwarf options.
> > 
> > commit dd216e07a156c19e589fc5f481fe13a256262801
> > gas: Use DW_FORM_sec_offset for DWARF version 4 or higher.
> > 
> > commit b0b3ea7e10443f211267babcac99ac6677d593ce
> > gas: Make sure to only add an md5 to a .file when requested.
> > 
> > commit bdd3b953e24c3d2bc55e06e30e0ef5e0ff829e3f
> > gas: Output .debug_rnglists for DWARF 5.
> > 
> > commit d2a54558074287721ce9e47142f7fa92bda15531
> > gas: Output directory and file names in .debug_line_str for DWARF5
> > 
> > commit edc7a80a9c04667dfa74f90f74bd39827bc372b7
> > gas: Don't error when .debug_line already exists, unless .loc was used
> > 
> > I can do the backports myself if you tell me how.
> 
> The how is quite straightforward.  You probably know most of this already
> but I am going to include it here just in case:
> 
>   0. Request permission to backport the patches.  
>      (Permission is granted for the ones listed above).

Turned out that the first two were already backported by you.
Sorry I missed that.

>   1. Checkout the mainline binutils sources from the git repository.
>   2. Get the patches for the commits above.
>   3. Switch to the binutils-2_35 branch.
>   4. Apply the patches.  
>      The Changelog changes will probably not apply cleanly, but this is
>      OK, because the convention is to create a new changelog entry with
>      today's date saying something along the lines of "backport this patch
>      from the mainline sources" and then including the original changelog
>      entry as a comment in the new changelog.

I did add a 'Backport from mainline:' header to the ChangeLog entries,
but note that if you hava the gnulib git-merge-changelog installed then
a git cherry-pick will automatically merge the ChangeLog entries.
https://gnu.wildebeest.org/blog/mjw/2012/03/16/automagically-merging-changelog-files-with-mercurial-or-git/

>   5. Test that the patched sources still build.

I tested all git cherry-picks individually and only had to adjust
commit bdd3b953e gas: Output .debug_rnglists for DWARF 5
for the change in argument order from the mainline commit e01e1cee3
Rearrange symbol_create parameters.

>   6. Commit the changes.
> 
> Once this is complete it would be worth sending an email to the list
> suggesting that the creation of a 2.35.1 release might be a good
> thing.  (Assuming of course that you do think that it would be a
> good thing).

I pushed the commits to the binutils-2_35-branch and do think it would
be nice to do a 2.35.1 release.

Thanks,

Mark