[v2,2/2] gas: re-work line number tracking for macros and their expansions

  The PR gas/16908 workaround aimed at uniformly reporting line numbers
to reference macro invocation sites. As mentioned in a comment this may
be desirable for small macros, but often isn't for larger ones. As a
first step improve diagnostics to report both locations, while aiming at
leaving generated debug info unaltered.

Note that macro invocation context is lost for any diagnostics issued
only after all input was processed (or more generally for any use of
as_*_where(), as the functions can't know whether the passed in location
is related to [part of] the present stack of locations). To maintain the
intended workaround behavior for PR gas/16908, a new as_where() is
introduced to "look through" macro invocations, while the existing
as_where() is renamed (and used in only very few places for now). Down
the road as_where() will likely want to return a list of (file,line)
pairs.
---
v2: Fully cover Arm testsuite.
---
Omitting testsuite adjustments from the inline patch, for its sheer size;
please see the attachment for the full (compressed) patch.
---
Adding

      if (subseg_text_p (now_seg))
	dwarf2_emit_insn (0);

to try_macro() (alongside using as_where_top() in dwarf2_where()) would
get .debug_line contents into reasonable shape (expressing both macro
invocation site and macro definition location). But that's likely
insufficient: We may want / need to represent macros as inline
subprograms and their expansions as inlined subroutines. Which in turn
looks cumbersome especially with the relatively recently added recording
of functions (with which macro expansions then would better be
associated, when marked as such).
  

Hi,

I'm not yet sure how, but this patch has broken at least one gdb test (gdb.asm/asm-source.exp) for aarch64-linux (Ubuntu 22.04 and 20.04).

Could you please take a look at it? I can do some testing with the aarch64 machines if that's helpful.

FAIL: gdb.asm/asm-source.exp: f at main
FAIL: gdb.asm/asm-source.exp: n at main
FAIL: gdb.asm/asm-source.exp: next over macro
FAIL: gdb.asm/asm-source.exp: list
FAIL: gdb.asm/asm-source.exp: f in foo2
FAIL: gdb.asm/asm-source.exp: n in foo2
FAIL: gdb.asm/asm-source.exp: bt ALL in foo2
FAIL: gdb.asm/asm-source.exp: bt 2 in foo2
FAIL: gdb.asm/asm-source.exp: bt 3 in foo3
FAIL: gdb.asm/asm-source.exp: finish from foo3
FAIL: gdb.asm/asm-source.exp: info source asmsrc2.s
FAIL: gdb.asm/asm-source.exp: info line
FAIL: gdb.asm/asm-source.exp: next over foo3
FAIL: gdb.asm/asm-source.exp: return from foo2

                 === gdb Summary ===

# of expected passes            15
# of unexpected failures        14

On 12/9/22 10:52, Jan Beulich via Binutils wrote:
> The PR gas/16908 workaround aimed at uniformly reporting line numbers
> to reference macro invocation sites. As mentioned in a comment this may
> be desirable for small macros, but often isn't for larger ones. As a
> first step improve diagnostics to report both locations, while aiming at
> leaving generated debug info unaltered.
> 
> Note that macro invocation context is lost for any diagnostics issued
> only after all input was processed (or more generally for any use of
> as_*_where(), as the functions can't know whether the passed in location
> is related to [part of] the present stack of locations). To maintain the
> intended workaround behavior for PR gas/16908, a new as_where() is
> introduced to "look through" macro invocations, while the existing
> as_where() is renamed (and used in only very few places for now). Down
> the road as_where() will likely want to return a list of (file,line)
> pairs.
> ---
> v2: Fully cover Arm testsuite.
> ---
> Omitting testsuite adjustments from the inline patch, for its sheer size;
> please see the attachment for the full (compressed) patch.
> ---
> Adding
> 
>        if (subseg_text_p (now_seg))
> 	dwarf2_emit_insn (0);
> 
> to try_macro() (alongside using as_where_top() in dwarf2_where()) would
> get .debug_line contents into reasonable shape (expressing both macro
> invocation site and macro definition location). But that's likely
> insufficient: We may want / need to represent macros as inline
> subprograms and their expansions as inlined subroutines. Which in turn
> looks cumbersome especially with the relatively recently added recording
> of functions (with which macro expansions then would better be
> associated, when marked as such).
> 
> --- a/gas/as.h
> +++ b/gas/as.h
> @@ -437,6 +437,10 @@ typedef struct _pseudo_type pseudo_typeS
>   #define PRINTF_WHERE_LIKE(FCN) \
>     void FCN (const char *file, unsigned int line, const char *format, ...) \
>       __attribute__ ((__format__ (__printf__, 3, 4)))
> +#define PRINTF_INDENT_LIKE(FCN) \
> +  void FCN (const char *file, unsigned int line, unsigned int indent, \
> +	    const char *format, ...) \
> +    __attribute__ ((__format__ (__printf__, 4, 5)))
>   
>   #else /* __GNUC__ < 2 || defined(VMS) */
>   
> @@ -444,6 +448,10 @@ typedef struct _pseudo_type pseudo_typeS
>   #define PRINTF_WHERE_LIKE(FCN)	void FCN (const char *file, \
>   					  unsigned int line, \
>   					  const char *format, ...)
> +#define PRINTF_INDENT_LIKE(FCN)	void FCN (const char *file, \
> +					  unsigned int line, \
> +					  unsigned int indent, \
> +					  const char *format, ...)
>   
>   #endif /* __GNUC__ < 2 || defined(VMS) */
>   
> @@ -453,6 +461,7 @@ PRINTF_LIKE (as_tsktsk);
>   PRINTF_LIKE (as_warn);
>   PRINTF_WHERE_LIKE (as_bad_where);
>   PRINTF_WHERE_LIKE (as_warn_where);
> +PRINTF_INDENT_LIKE (as_info_where);
>   
>   void   as_abort (const char *, int, const char *) ATTRIBUTE_NORETURN;
>   void   signal_init (void);
> @@ -487,7 +496,9 @@ void   cond_finish_check (int);
>   void   cond_exit_macro (int);
>   int    seen_at_least_1_file (void);
>   void   app_pop (char *);
> +void   as_report_context (void);
>   const char * as_where (unsigned int *);
> +const char * as_where_top (unsigned int *);
>   const char * as_where_physical (unsigned int *);
>   void   bump_line_counters (void);
>   void   do_scrub_begin (int);
> --- a/gas/input-scrub.c
> +++ b/gas/input-scrub.c
> @@ -104,6 +104,9 @@ static const char *logical_input_file;
>   static unsigned int physical_input_line;
>   static unsigned int logical_input_line;
>   
> +/* Indicator whether the origin of an update was a .linefile directive. */
> +static bool is_linefile;
> +
>   /* Struct used to save the state of the input handler during include files */
>   struct input_save {
>     char *              buffer_start;
> @@ -115,6 +118,7 @@ struct input_save {
>     const char *        logical_input_file;
>     unsigned int        physical_input_line;
>     unsigned int        logical_input_line;
> +  bool                is_linefile;
>     size_t              sb_index;
>     sb                  from_sb;
>     enum expansion      from_sb_expansion; /* Should we do a conditional check?  */
> @@ -166,6 +170,7 @@ input_scrub_push (char *saved_position)
>     saved->logical_input_file = logical_input_file;
>     saved->physical_input_line = physical_input_line;
>     saved->logical_input_line = logical_input_line;
> +  saved->is_linefile = is_linefile;
>     saved->sb_index = sb_index;
>     saved->from_sb = from_sb;
>     saved->from_sb_expansion = from_sb_expansion;
> @@ -193,6 +198,7 @@ input_scrub_pop (struct input_save *save
>     logical_input_file = saved->logical_input_file;
>     physical_input_line = saved->physical_input_line;
>     logical_input_line = saved->logical_input_line;
> +  is_linefile = saved->is_linefile;
>     sb_index = saved->sb_index;
>     from_sb = saved->from_sb;
>     from_sb_expansion = saved->from_sb_expansion;
> @@ -267,8 +273,6 @@ input_scrub_include_sb (sb *from, char *
>       as_fatal (_("macros nested too deeply"));
>     ++macro_nest;
>   
> -  gas_assert (expansion < expanding_nested);
> -
>   #ifdef md_macro_start
>     if (expansion == expanding_macro)
>       {
> @@ -283,8 +287,6 @@ input_scrub_include_sb (sb *from, char *
>        expansion.  */
>     newline = from->len >= 1 && from->ptr[0] != '\n';
>     sb_build (&from_sb, from->len + newline + 2 * sizeof (".linefile") + 30);
> -  if (expansion == expanding_repeat && from_sb_expansion >= expanding_macro)
> -    expansion = expanding_nested;
>     from_sb_expansion = expansion;
>     if (newline)
>       {
> @@ -437,10 +439,7 @@ bump_line_counters (void)
>     if (sb_index == (size_t) -1)
>       ++physical_input_line;
>   
> -  /* PR gas/16908 workaround: Don't bump logical line numbers while
> -     expanding macros, unless file (and maybe line; see as_where()) are
> -     used inside the macro.  */
> -  if (logical_input_line != -1u && from_sb_expansion < expanding_macro)
> +  if (logical_input_line != -1u)
>       ++logical_input_line;
>   }
>   
> @@ -471,10 +470,6 @@ new_logical_line_flags (const char *fnam
>       case 1 << 3:
>         if (line_number < 0 || fname != NULL)
>   	abort ();
> -      /* PR gas/16908 workaround: Ignore updates when nested inside a macro
> -	 expansion.  */
> -      if (from_sb_expansion == expanding_nested)
> -	return;
>         if (next_saved_file == NULL)
>   	fname = physical_input_file;
>         else if (next_saved_file->logical_input_file)
> @@ -486,6 +481,8 @@ new_logical_line_flags (const char *fnam
>         abort ();
>       }
>   
> +  is_linefile = flags != 1 && (flags != 0 || fname);
> +
>     if (line_number >= 0)
>       logical_input_line = line_number;
>     else if (line_number == -1 && fname && !*fname && (flags & (1 << 2)))
> @@ -499,15 +496,6 @@ new_logical_line_flags (const char *fnam
>         && (logical_input_file == NULL
>   	  || filename_cmp (logical_input_file, fname)))
>       logical_input_file = fname;
> -
> -  /* When encountering file or line changes inside a macro, arrange for
> -     bump_line_counters() to henceforth increment the logical line number
> -     again, just like it does when expanding repeats.  See as_where() for
> -     why changing file or line alone doesn't alter expansion mode.  */
> -  if (from_sb_expansion == expanding_macro
> -      && logical_input_file != NULL
> -      && logical_input_line != -1u)
> -    from_sb_expansion = expanding_repeat;
>   }
>   
>   void
> @@ -516,6 +504,33 @@ new_logical_line (const char *fname, int
>     new_logical_line_flags (fname, line_number, 0);
>   }
>   
> +void
> +as_report_context (void)
> +{
> +  const struct input_save *saved = next_saved_file;
> +  enum expansion expansion = from_sb_expansion;
> +  int indent = 1;
> +
> +  if (!macro_nest)
> +    return;
> +
> +  do
> +    {
> +      if (expansion != expanding_macro)
> +	/* Nothing.  */;
> +      else if (saved->logical_input_file != NULL
> +	       && saved->logical_input_line != -1u)
> +	as_info_where (saved->logical_input_file, saved->logical_input_line,
> +		       indent, _("macro invoked from here"));
> +      else
> +	as_info_where (saved->physical_input_file, saved->physical_input_line,
> +		       indent, _("macro invoked from here"));
> +
> +      expansion = saved->from_sb_expansion;
> +      ++indent;
> +    }
> +  while ((saved = saved->next_saved_file) != NULL);
> +}
>   
>   /* Return the current physical input file name and line number, if known  */
>   
> @@ -534,11 +549,53 @@ as_where_physical (unsigned int *linep)
>     return NULL;
>   }
>   
> -/* Return the current file name and line number.  */
> +/* Return the file name and line number at the top most macro
> +   invocation, unless .file / .line were used inside a macro.  */
>   
>   const char *
>   as_where (unsigned int *linep)
>   {
> +  const char *file = as_where_top (linep);
> +
> +  if (macro_nest && is_linefile)
> +    {
> +      const struct input_save *saved = next_saved_file;
> +      enum expansion expansion = from_sb_expansion;
> +
> +      do
> +	{
> +	  if (!saved->is_linefile)
> +	    break;
> +
> +	  if (expansion != expanding_macro)
> +	    /* Nothing.  */;
> +	  else if (saved->logical_input_file != NULL
> +		   && (linep == NULL || saved->logical_input_line != -1u))
> +	    {
> +	      if (linep != NULL)
> +		*linep = saved->logical_input_line;
> +	      file = saved->logical_input_file;
> +	    }
> +	  else if (saved->physical_input_file != NULL)
> +	    {
> +	      if (linep != NULL)
> +		*linep = saved->physical_input_line;
> +	      file = saved->physical_input_file;
> +	    }
> +
> +	  expansion = saved->from_sb_expansion;
> +	}
> +      while ((saved = saved->next_saved_file) != NULL);
> +    }
> +
> +  return file;
> +}
> +
> +/* Return the current file name and line number.  */
> +
> +const char *
> +as_where_top (unsigned int *linep)
> +{
>     if (logical_input_file != NULL
>         && (linep == NULL || logical_input_line != -1u))
>       {
> @@ -549,4 +606,3 @@ as_where (unsigned int *linep)
>   
>     return as_where_physical (linep);
>   }
> -
> --- a/gas/macro.c
> +++ b/gas/macro.c
> @@ -131,23 +131,21 @@ buffer_and_nest (const char *from, const
>     else
>       from_len = strlen (from);
>   
> -  /* Except for macros record the present source position, such that
> -     diagnostics and debug info will be properly associated with the
> -     respective original lines, rather than with the line of the ending
> -     directive (TO).  */
> -  if (from == NULL || strcasecmp (from, "MACRO") != 0)
> -    {
> -      unsigned int line;
> -      char *linefile;
> -
> -      as_where (&line);
> -      if (!flag_m68k_mri)
> -	linefile = xasprintf ("\t.linefile %u .", line + 1);
> -      else
> -	linefile = xasprintf ("\tlinefile %u .", line + 1);
> -      sb_add_string (ptr, linefile);
> -      xfree (linefile);
> -    }
> +  /* Record the present source position, such that diagnostics and debug info
> +     can be properly associated with the respective original lines, rather
> +     than with the line of the ending directive (TO).  */
> +  {
> +    unsigned int line;
> +    char *linefile;
> +
> +    as_where_top (&line);
> +    if (!flag_m68k_mri)
> +      linefile = xasprintf ("\t.linefile %u .", line + 1);
> +    else
> +      linefile = xasprintf ("\tlinefile %u .", line + 1);
> +    sb_add_string (ptr, linefile);
> +    xfree (linefile);
> +  }
>   
>     while (more)
>       {
> @@ -249,14 +247,8 @@ buffer_and_nest (const char *from, const
>   	    }
>   
>   	  /* PR gas/16908
> -	     Apply and discard .linefile directives that appear within
> -	     the macro.  For long macros, one might want to report the
> -	     line number information associated with the lines within
> -	     the macro definition, but we would need more infrastructure
> -	     to make that happen correctly (e.g. resetting the line
> -	     number when expanding the macro), and since for short
> -	     macros we clearly prefer reporting the point of expansion
> -	     anyway, there's not an obviously better fix here.  */
> +	     Apply .linefile directives that appear within the macro, alongside
> +	     keeping them for later expansion of the macro.  */
>   	  if (from != NULL && strcasecmp (from, "MACRO") == 0
>   	      && len >= 8 && strncasecmp (ptr->ptr + i, "linefile", 8) == 0)
>   	    {
> @@ -267,7 +259,6 @@ buffer_and_nest (const char *from, const
>   	      s_linefile (0);
>   	      restore_ilp ();
>   	      ptr->ptr[ptr->len] = saved_eol_char;
> -	      ptr->len = line_start;
>   	    }
>   	}
>   
> --- a/gas/messages.c
> +++ b/gas/messages.c
> @@ -18,6 +18,7 @@
>      02110-1301, USA.  */
>   
>   #include "as.h"
> +#include <limits.h>
>   #include <signal.h>
>   
>   /* If the system doesn't provide strsignal, we get it defined in
> @@ -119,7 +120,7 @@ as_show_where (void)
>     const char *file;
>     unsigned int line;
>   
> -  file = as_where (&line);
> +  file = as_where_top (&line);
>     identify (file);
>     if (file)
>       {
> @@ -130,6 +131,25 @@ as_show_where (void)
>       }
>   }
>   
> +/* Send to stderr a string as information, with location data passed in.
> +   Note that for now this is not intended for general use.  */
> +
> +void
> +as_info_where (const char *file, unsigned int line, unsigned int indent,
> +	       const char *format, ...)
> +{
> +  va_list args;
> +  char buffer[2000];
> +
> +  gas_assert (file != NULL && line > 0 && indent <= INT_MAX);
> +
> +  va_start (args, format);
> +  vsnprintf (buffer, sizeof (buffer), format, args);
> +  va_end (args);
> +  fprintf (stderr, "%s:%u: %*s%s%s\n",
> +	   file, line, (int)indent, "", _("Info: "), buffer);
> +}
> +
>   /* Send to stderr a string as a warning, and locate warning
>      in input file(s).
>      Please only use this for when we have some recovery action.
> @@ -146,6 +166,7 @@ as_tsktsk (const char *format, ...)
>     vfprintf (stderr, format, args);
>     va_end (args);
>     (void) putc ('\n', stderr);
> +  as_report_context ();
>   }
>   
>   /* The common portion of as_warn and as_warn_where.  */
> @@ -153,10 +174,15 @@ as_tsktsk (const char *format, ...)
>   static void
>   as_warn_internal (const char *file, unsigned int line, char *buffer)
>   {
> +  bool context = false;
> +
>     ++warning_count;
>   
>     if (file == NULL)
> -    file = as_where (&line);
> +    {
> +      file = as_where_top (&line);
> +      context = true;
> +    }
>   
>     identify (file);
>     if (file)
> @@ -168,6 +194,10 @@ as_warn_internal (const char *file, unsi
>       }
>     else
>       fprintf (stderr, "%s%s\n", _("Warning: "), buffer);
> +
> +  if (context)
> +    as_report_context ();
> +
>   #ifndef NO_LISTING
>     listing_warning (buffer);
>   #endif
> @@ -194,7 +224,7 @@ as_warn (const char *format, ...)
>       }
>   }
>   
> -/* Like as_bad but the file name and line number are passed in.
> +/* Like as_warn but the file name and line number are passed in.
>      Unfortunately, we have to repeat the function in order to handle
>      the varargs correctly and portably.  */
>   
> @@ -218,10 +248,15 @@ as_warn_where (const char *file, unsigne
>   static void
>   as_bad_internal (const char *file, unsigned int line, char *buffer)
>   {
> +  bool context = false;
> +
>     ++error_count;
>   
>     if (file == NULL)
> -    file = as_where (&line);
> +    {
> +      file = as_where_top (&line);
> +      context = true;
> +    }
>   
>     identify (file);
>     if (file)
> @@ -233,6 +268,10 @@ as_bad_internal (const char *file, unsig
>       }
>     else
>       fprintf (stderr, "%s%s\n", _("Error: "), buffer);
> +
> +  if (context)
> +    as_report_context ();
> +
>   #ifndef NO_LISTING
>     listing_error (buffer);
>   #endif
> @@ -290,6 +329,7 @@ as_fatal (const char *format, ...)
>     vfprintf (stderr, format, args);
>     (void) putc ('\n', stderr);
>     va_end (args);
> +  as_report_context ();
>     /* Delete the output file, if it exists.  This will prevent make from
>        thinking that a file was created and hence does not need rebuilding.  */
>     if (out_file_name != NULL)
> @@ -312,6 +352,7 @@ as_abort (const char *file, int line, co
>       fprintf (stderr, _("Internal error in %s at %s:%d.\n"), fn, file, line);
>     else
>       fprintf (stderr, _("Internal error at %s:%d.\n"), file, line);
> +  as_report_context ();
>   
>     fprintf (stderr, _("Please report this bug.\n"));
>   
> --- a/gas/sb.h
> +++ b/gas/sb.h
> @@ -66,11 +66,9 @@ extern size_t sb_skip_comma (size_t, sb
>   
>   /* Actually in input-scrub.c.  */
>   enum expansion {
> -  /* Note: Order matters!  */
>     expanding_none,
>     expanding_repeat,
>     expanding_macro,
> -  expanding_nested, /* Only for internal use of input-scrub.c.  */
>   };
>   extern void input_scrub_include_sb (sb *, char *, enum expansion);
>
  

On 12/14/22 14:03, Luis Machado via Binutils wrote:
> Hi,
> 
> I'm not yet sure how, but this patch has broken at least one gdb test (gdb.asm/asm-source.exp) for aarch64-linux (Ubuntu 22.04 and 20.04).
> 
> Could you please take a look at it? I can do some testing with the aarch64 machines if that's helpful.
> 
> FAIL: gdb.asm/asm-source.exp: f at main
> FAIL: gdb.asm/asm-source.exp: n at main
> FAIL: gdb.asm/asm-source.exp: next over macro
> FAIL: gdb.asm/asm-source.exp: list
> FAIL: gdb.asm/asm-source.exp: f in foo2
> FAIL: gdb.asm/asm-source.exp: n in foo2
> FAIL: gdb.asm/asm-source.exp: bt ALL in foo2
> FAIL: gdb.asm/asm-source.exp: bt 2 in foo2
> FAIL: gdb.asm/asm-source.exp: bt 3 in foo3
> FAIL: gdb.asm/asm-source.exp: finish from foo3
> FAIL: gdb.asm/asm-source.exp: info source asmsrc2.s
> FAIL: gdb.asm/asm-source.exp: info line
> FAIL: gdb.asm/asm-source.exp: next over foo3
> FAIL: gdb.asm/asm-source.exp: return from foo2
> 
>                  === gdb Summary ===
> 
> # of expected passes            15
> # of unexpected failures        14
> 

I'm guessing if we build gas alongside gdb, then the gdb testsuite will use that new gas binary.

If something has changed in the format/structure of the output, we may need to adjust the testcase to match.
  

Luis Machado via Binutils <binutils@sourceware.org> writes:

> Hi,
>
> I'm not yet sure how, but this patch has broken at least one gdb test (gdb.asm/asm-source.exp) for aarch64-linux (Ubuntu 22.04 and 20.04).
>
> Could you please take a look at it? I can do some testing with the aarch64 machines if that's helpful.
>
> FAIL: gdb.asm/asm-source.exp: f at main
> FAIL: gdb.asm/asm-source.exp: n at main
> FAIL: gdb.asm/asm-source.exp: next over macro
> FAIL: gdb.asm/asm-source.exp: list
> FAIL: gdb.asm/asm-source.exp: f in foo2
> FAIL: gdb.asm/asm-source.exp: n in foo2
> FAIL: gdb.asm/asm-source.exp: bt ALL in foo2
> FAIL: gdb.asm/asm-source.exp: bt 2 in foo2
> FAIL: gdb.asm/asm-source.exp: bt 3 in foo3
> FAIL: gdb.asm/asm-source.exp: finish from foo3
> FAIL: gdb.asm/asm-source.exp: info source asmsrc2.s
> FAIL: gdb.asm/asm-source.exp: info line
> FAIL: gdb.asm/asm-source.exp: next over foo3
> FAIL: gdb.asm/asm-source.exp: return from foo2

Can confirm I'm seeing the same set of failures on x86-64 gdb when using
gas that includes this patch.

Thanks,
Andrew


>
>                  === gdb Summary ===
>
> # of expected passes            15
> # of unexpected failures        14
>
> On 12/9/22 10:52, Jan Beulich via Binutils wrote:
>> The PR gas/16908 workaround aimed at uniformly reporting line numbers
>> to reference macro invocation sites. As mentioned in a comment this may
>> be desirable for small macros, but often isn't for larger ones. As a
>> first step improve diagnostics to report both locations, while aiming at
>> leaving generated debug info unaltered.
>> 
>> Note that macro invocation context is lost for any diagnostics issued
>> only after all input was processed (or more generally for any use of
>> as_*_where(), as the functions can't know whether the passed in location
>> is related to [part of] the present stack of locations). To maintain the
>> intended workaround behavior for PR gas/16908, a new as_where() is
>> introduced to "look through" macro invocations, while the existing
>> as_where() is renamed (and used in only very few places for now). Down
>> the road as_where() will likely want to return a list of (file,line)
>> pairs.
>> ---
>> v2: Fully cover Arm testsuite.
>> ---
>> Omitting testsuite adjustments from the inline patch, for its sheer size;
>> please see the attachment for the full (compressed) patch.
>> ---
>> Adding
>> 
>>        if (subseg_text_p (now_seg))
>> 	dwarf2_emit_insn (0);
>> 
>> to try_macro() (alongside using as_where_top() in dwarf2_where()) would
>> get .debug_line contents into reasonable shape (expressing both macro
>> invocation site and macro definition location). But that's likely
>> insufficient: We may want / need to represent macros as inline
>> subprograms and their expansions as inlined subroutines. Which in turn
>> looks cumbersome especially with the relatively recently added recording
>> of functions (with which macro expansions then would better be
>> associated, when marked as such).
>> 
>> --- a/gas/as.h
>> +++ b/gas/as.h
>> @@ -437,6 +437,10 @@ typedef struct _pseudo_type pseudo_typeS
>>   #define PRINTF_WHERE_LIKE(FCN) \
>>     void FCN (const char *file, unsigned int line, const char *format, ...) \
>>       __attribute__ ((__format__ (__printf__, 3, 4)))
>> +#define PRINTF_INDENT_LIKE(FCN) \
>> +  void FCN (const char *file, unsigned int line, unsigned int indent, \
>> +	    const char *format, ...) \
>> +    __attribute__ ((__format__ (__printf__, 4, 5)))
>>   
>>   #else /* __GNUC__ < 2 || defined(VMS) */
>>   
>> @@ -444,6 +448,10 @@ typedef struct _pseudo_type pseudo_typeS
>>   #define PRINTF_WHERE_LIKE(FCN)	void FCN (const char *file, \
>>   					  unsigned int line, \
>>   					  const char *format, ...)
>> +#define PRINTF_INDENT_LIKE(FCN)	void FCN (const char *file, \
>> +					  unsigned int line, \
>> +					  unsigned int indent, \
>> +					  const char *format, ...)
>>   
>>   #endif /* __GNUC__ < 2 || defined(VMS) */
>>   
>> @@ -453,6 +461,7 @@ PRINTF_LIKE (as_tsktsk);
>>   PRINTF_LIKE (as_warn);
>>   PRINTF_WHERE_LIKE (as_bad_where);
>>   PRINTF_WHERE_LIKE (as_warn_where);
>> +PRINTF_INDENT_LIKE (as_info_where);
>>   
>>   void   as_abort (const char *, int, const char *) ATTRIBUTE_NORETURN;
>>   void   signal_init (void);
>> @@ -487,7 +496,9 @@ void   cond_finish_check (int);
>>   void   cond_exit_macro (int);
>>   int    seen_at_least_1_file (void);
>>   void   app_pop (char *);
>> +void   as_report_context (void);
>>   const char * as_where (unsigned int *);
>> +const char * as_where_top (unsigned int *);
>>   const char * as_where_physical (unsigned int *);
>>   void   bump_line_counters (void);
>>   void   do_scrub_begin (int);
>> --- a/gas/input-scrub.c
>> +++ b/gas/input-scrub.c
>> @@ -104,6 +104,9 @@ static const char *logical_input_file;
>>   static unsigned int physical_input_line;
>>   static unsigned int logical_input_line;
>>   
>> +/* Indicator whether the origin of an update was a .linefile directive. */
>> +static bool is_linefile;
>> +
>>   /* Struct used to save the state of the input handler during include files */
>>   struct input_save {
>>     char *              buffer_start;
>> @@ -115,6 +118,7 @@ struct input_save {
>>     const char *        logical_input_file;
>>     unsigned int        physical_input_line;
>>     unsigned int        logical_input_line;
>> +  bool                is_linefile;
>>     size_t              sb_index;
>>     sb                  from_sb;
>>     enum expansion      from_sb_expansion; /* Should we do a conditional check?  */
>> @@ -166,6 +170,7 @@ input_scrub_push (char *saved_position)
>>     saved->logical_input_file = logical_input_file;
>>     saved->physical_input_line = physical_input_line;
>>     saved->logical_input_line = logical_input_line;
>> +  saved->is_linefile = is_linefile;
>>     saved->sb_index = sb_index;
>>     saved->from_sb = from_sb;
>>     saved->from_sb_expansion = from_sb_expansion;
>> @@ -193,6 +198,7 @@ input_scrub_pop (struct input_save *save
>>     logical_input_file = saved->logical_input_file;
>>     physical_input_line = saved->physical_input_line;
>>     logical_input_line = saved->logical_input_line;
>> +  is_linefile = saved->is_linefile;
>>     sb_index = saved->sb_index;
>>     from_sb = saved->from_sb;
>>     from_sb_expansion = saved->from_sb_expansion;
>> @@ -267,8 +273,6 @@ input_scrub_include_sb (sb *from, char *
>>       as_fatal (_("macros nested too deeply"));
>>     ++macro_nest;
>>   
>> -  gas_assert (expansion < expanding_nested);
>> -
>>   #ifdef md_macro_start
>>     if (expansion == expanding_macro)
>>       {
>> @@ -283,8 +287,6 @@ input_scrub_include_sb (sb *from, char *
>>        expansion.  */
>>     newline = from->len >= 1 && from->ptr[0] != '\n';
>>     sb_build (&from_sb, from->len + newline + 2 * sizeof (".linefile") + 30);
>> -  if (expansion == expanding_repeat && from_sb_expansion >= expanding_macro)
>> -    expansion = expanding_nested;
>>     from_sb_expansion = expansion;
>>     if (newline)
>>       {
>> @@ -437,10 +439,7 @@ bump_line_counters (void)
>>     if (sb_index == (size_t) -1)
>>       ++physical_input_line;
>>   
>> -  /* PR gas/16908 workaround: Don't bump logical line numbers while
>> -     expanding macros, unless file (and maybe line; see as_where()) are
>> -     used inside the macro.  */
>> -  if (logical_input_line != -1u && from_sb_expansion < expanding_macro)
>> +  if (logical_input_line != -1u)
>>       ++logical_input_line;
>>   }
>>   
>> @@ -471,10 +470,6 @@ new_logical_line_flags (const char *fnam
>>       case 1 << 3:
>>         if (line_number < 0 || fname != NULL)
>>   	abort ();
>> -      /* PR gas/16908 workaround: Ignore updates when nested inside a macro
>> -	 expansion.  */
>> -      if (from_sb_expansion == expanding_nested)
>> -	return;
>>         if (next_saved_file == NULL)
>>   	fname = physical_input_file;
>>         else if (next_saved_file->logical_input_file)
>> @@ -486,6 +481,8 @@ new_logical_line_flags (const char *fnam
>>         abort ();
>>       }
>>   
>> +  is_linefile = flags != 1 && (flags != 0 || fname);
>> +
>>     if (line_number >= 0)
>>       logical_input_line = line_number;
>>     else if (line_number == -1 && fname && !*fname && (flags & (1 << 2)))
>> @@ -499,15 +496,6 @@ new_logical_line_flags (const char *fnam
>>         && (logical_input_file == NULL
>>   	  || filename_cmp (logical_input_file, fname)))
>>       logical_input_file = fname;
>> -
>> -  /* When encountering file or line changes inside a macro, arrange for
>> -     bump_line_counters() to henceforth increment the logical line number
>> -     again, just like it does when expanding repeats.  See as_where() for
>> -     why changing file or line alone doesn't alter expansion mode.  */
>> -  if (from_sb_expansion == expanding_macro
>> -      && logical_input_file != NULL
>> -      && logical_input_line != -1u)
>> -    from_sb_expansion = expanding_repeat;
>>   }
>>   
>>   void
>> @@ -516,6 +504,33 @@ new_logical_line (const char *fname, int
>>     new_logical_line_flags (fname, line_number, 0);
>>   }
>>   
>> +void
>> +as_report_context (void)
>> +{
>> +  const struct input_save *saved = next_saved_file;
>> +  enum expansion expansion = from_sb_expansion;
>> +  int indent = 1;
>> +
>> +  if (!macro_nest)
>> +    return;
>> +
>> +  do
>> +    {
>> +      if (expansion != expanding_macro)
>> +	/* Nothing.  */;
>> +      else if (saved->logical_input_file != NULL
>> +	       && saved->logical_input_line != -1u)
>> +	as_info_where (saved->logical_input_file, saved->logical_input_line,
>> +		       indent, _("macro invoked from here"));
>> +      else
>> +	as_info_where (saved->physical_input_file, saved->physical_input_line,
>> +		       indent, _("macro invoked from here"));
>> +
>> +      expansion = saved->from_sb_expansion;
>> +      ++indent;
>> +    }
>> +  while ((saved = saved->next_saved_file) != NULL);
>> +}
>>   
>>   /* Return the current physical input file name and line number, if known  */
>>   
>> @@ -534,11 +549,53 @@ as_where_physical (unsigned int *linep)
>>     return NULL;
>>   }
>>   
>> -/* Return the current file name and line number.  */
>> +/* Return the file name and line number at the top most macro
>> +   invocation, unless .file / .line were used inside a macro.  */
>>   
>>   const char *
>>   as_where (unsigned int *linep)
>>   {
>> +  const char *file = as_where_top (linep);
>> +
>> +  if (macro_nest && is_linefile)
>> +    {
>> +      const struct input_save *saved = next_saved_file;
>> +      enum expansion expansion = from_sb_expansion;
>> +
>> +      do
>> +	{
>> +	  if (!saved->is_linefile)
>> +	    break;
>> +
>> +	  if (expansion != expanding_macro)
>> +	    /* Nothing.  */;
>> +	  else if (saved->logical_input_file != NULL
>> +		   && (linep == NULL || saved->logical_input_line != -1u))
>> +	    {
>> +	      if (linep != NULL)
>> +		*linep = saved->logical_input_line;
>> +	      file = saved->logical_input_file;
>> +	    }
>> +	  else if (saved->physical_input_file != NULL)
>> +	    {
>> +	      if (linep != NULL)
>> +		*linep = saved->physical_input_line;
>> +	      file = saved->physical_input_file;
>> +	    }
>> +
>> +	  expansion = saved->from_sb_expansion;
>> +	}
>> +      while ((saved = saved->next_saved_file) != NULL);
>> +    }
>> +
>> +  return file;
>> +}
>> +
>> +/* Return the current file name and line number.  */
>> +
>> +const char *
>> +as_where_top (unsigned int *linep)
>> +{
>>     if (logical_input_file != NULL
>>         && (linep == NULL || logical_input_line != -1u))
>>       {
>> @@ -549,4 +606,3 @@ as_where (unsigned int *linep)
>>   
>>     return as_where_physical (linep);
>>   }
>> -
>> --- a/gas/macro.c
>> +++ b/gas/macro.c
>> @@ -131,23 +131,21 @@ buffer_and_nest (const char *from, const
>>     else
>>       from_len = strlen (from);
>>   
>> -  /* Except for macros record the present source position, such that
>> -     diagnostics and debug info will be properly associated with the
>> -     respective original lines, rather than with the line of the ending
>> -     directive (TO).  */
>> -  if (from == NULL || strcasecmp (from, "MACRO") != 0)
>> -    {
>> -      unsigned int line;
>> -      char *linefile;
>> -
>> -      as_where (&line);
>> -      if (!flag_m68k_mri)
>> -	linefile = xasprintf ("\t.linefile %u .", line + 1);
>> -      else
>> -	linefile = xasprintf ("\tlinefile %u .", line + 1);
>> -      sb_add_string (ptr, linefile);
>> -      xfree (linefile);
>> -    }
>> +  /* Record the present source position, such that diagnostics and debug info
>> +     can be properly associated with the respective original lines, rather
>> +     than with the line of the ending directive (TO).  */
>> +  {
>> +    unsigned int line;
>> +    char *linefile;
>> +
>> +    as_where_top (&line);
>> +    if (!flag_m68k_mri)
>> +      linefile = xasprintf ("\t.linefile %u .", line + 1);
>> +    else
>> +      linefile = xasprintf ("\tlinefile %u .", line + 1);
>> +    sb_add_string (ptr, linefile);
>> +    xfree (linefile);
>> +  }
>>   
>>     while (more)
>>       {
>> @@ -249,14 +247,8 @@ buffer_and_nest (const char *from, const
>>   	    }
>>   
>>   	  /* PR gas/16908
>> -	     Apply and discard .linefile directives that appear within
>> -	     the macro.  For long macros, one might want to report the
>> -	     line number information associated with the lines within
>> -	     the macro definition, but we would need more infrastructure
>> -	     to make that happen correctly (e.g. resetting the line
>> -	     number when expanding the macro), and since for short
>> -	     macros we clearly prefer reporting the point of expansion
>> -	     anyway, there's not an obviously better fix here.  */
>> +	     Apply .linefile directives that appear within the macro, alongside
>> +	     keeping them for later expansion of the macro.  */
>>   	  if (from != NULL && strcasecmp (from, "MACRO") == 0
>>   	      && len >= 8 && strncasecmp (ptr->ptr + i, "linefile", 8) == 0)
>>   	    {
>> @@ -267,7 +259,6 @@ buffer_and_nest (const char *from, const
>>   	      s_linefile (0);
>>   	      restore_ilp ();
>>   	      ptr->ptr[ptr->len] = saved_eol_char;
>> -	      ptr->len = line_start;
>>   	    }
>>   	}
>>   
>> --- a/gas/messages.c
>> +++ b/gas/messages.c
>> @@ -18,6 +18,7 @@
>>      02110-1301, USA.  */
>>   
>>   #include "as.h"
>> +#include <limits.h>
>>   #include <signal.h>
>>   
>>   /* If the system doesn't provide strsignal, we get it defined in
>> @@ -119,7 +120,7 @@ as_show_where (void)
>>     const char *file;
>>     unsigned int line;
>>   
>> -  file = as_where (&line);
>> +  file = as_where_top (&line);
>>     identify (file);
>>     if (file)
>>       {
>> @@ -130,6 +131,25 @@ as_show_where (void)
>>       }
>>   }
>>   
>> +/* Send to stderr a string as information, with location data passed in.
>> +   Note that for now this is not intended for general use.  */
>> +
>> +void
>> +as_info_where (const char *file, unsigned int line, unsigned int indent,
>> +	       const char *format, ...)
>> +{
>> +  va_list args;
>> +  char buffer[2000];
>> +
>> +  gas_assert (file != NULL && line > 0 && indent <= INT_MAX);
>> +
>> +  va_start (args, format);
>> +  vsnprintf (buffer, sizeof (buffer), format, args);
>> +  va_end (args);
>> +  fprintf (stderr, "%s:%u: %*s%s%s\n",
>> +	   file, line, (int)indent, "", _("Info: "), buffer);
>> +}
>> +
>>   /* Send to stderr a string as a warning, and locate warning
>>      in input file(s).
>>      Please only use this for when we have some recovery action.
>> @@ -146,6 +166,7 @@ as_tsktsk (const char *format, ...)
>>     vfprintf (stderr, format, args);
>>     va_end (args);
>>     (void) putc ('\n', stderr);
>> +  as_report_context ();
>>   }
>>   
>>   /* The common portion of as_warn and as_warn_where.  */
>> @@ -153,10 +174,15 @@ as_tsktsk (const char *format, ...)
>>   static void
>>   as_warn_internal (const char *file, unsigned int line, char *buffer)
>>   {
>> +  bool context = false;
>> +
>>     ++warning_count;
>>   
>>     if (file == NULL)
>> -    file = as_where (&line);
>> +    {
>> +      file = as_where_top (&line);
>> +      context = true;
>> +    }
>>   
>>     identify (file);
>>     if (file)
>> @@ -168,6 +194,10 @@ as_warn_internal (const char *file, unsi
>>       }
>>     else
>>       fprintf (stderr, "%s%s\n", _("Warning: "), buffer);
>> +
>> +  if (context)
>> +    as_report_context ();
>> +
>>   #ifndef NO_LISTING
>>     listing_warning (buffer);
>>   #endif
>> @@ -194,7 +224,7 @@ as_warn (const char *format, ...)
>>       }
>>   }
>>   
>> -/* Like as_bad but the file name and line number are passed in.
>> +/* Like as_warn but the file name and line number are passed in.
>>      Unfortunately, we have to repeat the function in order to handle
>>      the varargs correctly and portably.  */
>>   
>> @@ -218,10 +248,15 @@ as_warn_where (const char *file, unsigne
>>   static void
>>   as_bad_internal (const char *file, unsigned int line, char *buffer)
>>   {
>> +  bool context = false;
>> +
>>     ++error_count;
>>   
>>     if (file == NULL)
>> -    file = as_where (&line);
>> +    {
>> +      file = as_where_top (&line);
>> +      context = true;
>> +    }
>>   
>>     identify (file);
>>     if (file)
>> @@ -233,6 +268,10 @@ as_bad_internal (const char *file, unsig
>>       }
>>     else
>>       fprintf (stderr, "%s%s\n", _("Error: "), buffer);
>> +
>> +  if (context)
>> +    as_report_context ();
>> +
>>   #ifndef NO_LISTING
>>     listing_error (buffer);
>>   #endif
>> @@ -290,6 +329,7 @@ as_fatal (const char *format, ...)
>>     vfprintf (stderr, format, args);
>>     (void) putc ('\n', stderr);
>>     va_end (args);
>> +  as_report_context ();
>>     /* Delete the output file, if it exists.  This will prevent make from
>>        thinking that a file was created and hence does not need rebuilding.  */
>>     if (out_file_name != NULL)
>> @@ -312,6 +352,7 @@ as_abort (const char *file, int line, co
>>       fprintf (stderr, _("Internal error in %s at %s:%d.\n"), fn, file, line);
>>     else
>>       fprintf (stderr, _("Internal error at %s:%d.\n"), file, line);
>> +  as_report_context ();
>>   
>>     fprintf (stderr, _("Please report this bug.\n"));
>>   
>> --- a/gas/sb.h
>> +++ b/gas/sb.h
>> @@ -66,11 +66,9 @@ extern size_t sb_skip_comma (size_t, sb
>>   
>>   /* Actually in input-scrub.c.  */
>>   enum expansion {
>> -  /* Note: Order matters!  */
>>     expanding_none,
>>     expanding_repeat,
>>     expanding_macro,
>> -  expanding_nested, /* Only for internal use of input-scrub.c.  */
>>   };
>>   extern void input_scrub_include_sb (sb *, char *, enum expansion);
>>
  

Andrew Burgess <aburgess@redhat.com> writes:

> Luis Machado via Binutils <binutils@sourceware.org> writes:
>
>> Hi,
>>
>> I'm not yet sure how, but this patch has broken at least one gdb test (gdb.asm/asm-source.exp) for aarch64-linux (Ubuntu 22.04 and 20.04).
>>
>> Could you please take a look at it? I can do some testing with the aarch64 machines if that's helpful.
>>
>> FAIL: gdb.asm/asm-source.exp: f at main
>> FAIL: gdb.asm/asm-source.exp: n at main
>> FAIL: gdb.asm/asm-source.exp: next over macro
>> FAIL: gdb.asm/asm-source.exp: list
>> FAIL: gdb.asm/asm-source.exp: f in foo2
>> FAIL: gdb.asm/asm-source.exp: n in foo2
>> FAIL: gdb.asm/asm-source.exp: bt ALL in foo2
>> FAIL: gdb.asm/asm-source.exp: bt 2 in foo2
>> FAIL: gdb.asm/asm-source.exp: bt 3 in foo3
>> FAIL: gdb.asm/asm-source.exp: finish from foo3
>> FAIL: gdb.asm/asm-source.exp: info source asmsrc2.s
>> FAIL: gdb.asm/asm-source.exp: info line
>> FAIL: gdb.asm/asm-source.exp: next over foo3
>> FAIL: gdb.asm/asm-source.exp: return from foo2
>
> Can confirm I'm seeing the same set of failures on x86-64 gdb when using
> gas that includes this patch.

I think I understand the issue a little more now.  I have a simple
reproducer which can be run outside the gdb testsuite (see below).

It appears that the DWARF for macros now tries to associate the
instructions within the macro the source location within the macro
definition, rather than the macro use site.  I'm not entirely convinced
this is a good idea (as a macro could be used multiple times), or even
if this was an intended change of this series.

If this is the direction gas is moving in then I guess we will need to
update the GDB test, but there is, I think, a bug in the generated
DWARF, in that it appears that the wrong file name is being used.

Here's my steps to reproduce:

  $ cat test.s 
          .include "other.inc"
  
          .global _start
  _start:
          xor	%rbp, %rbp
          call    main
          hlt
  
          .global main
  main:
          gdbasm_enter
          hlt
  $ cat other.inc 
  	.macro gdbasm_enter
  	push	%rbp
  	mov	%rsp,%rbp
  	.endm
  $ cat Makefile 
  all:
  	$(AS) --gdwarf-2 -o test.o test.s
  	$(LD) -o test test.o
  
  clean:
  	-rm -f test.o test
  
  $ make AS=/path/to/recent/build/of/gas/as-new
  /path/to/recent/build/of/gas/as-new --gdwarf-2 -o test.o test.s
  ld -o test test.o
  $ cat cmd.gdb 
  set height 0
  set trace-commands on
  start
  frame
  
  $ gdb -q -x cmd.gdb test
  Reading symbols from test...
  +start
  Temporary breakpoint 1 at 0x401009: file test.s, line 2.
  
  Temporary breakpoint 1, main () at test.s:2
  2	
  +frame
  #0  main () at test.s:2
  2	
  (gdb)

Notice that when we stopped at 'main' in gdb the location was reported
as test.s:2.  The '2' here is correctly indicating line 2, but the file
should be 'other.inc', not 'test.s'.

If I rebuild using an older version of gas, then repeat the gdb step,
the old behaviour was:

  $ make
  as --gdwarf-2 -o test.o test.s
  ld -o test test.o
  $ gdb -q -x cmd.gdb test
  Reading symbols from test...
  +start
  Temporary breakpoint 1 at 0x401009: file test.s, line 11.
  
  Temporary breakpoint 1, main () at test.s:11
  11	        gdbasm_enter
  +frame
  #0  main () at test.s:11
  11	        gdbasm_enter
  (gdb) 

Now the location is 'test.s:11', pointing at the macro invocation.  To
me, this seems more helpful, but that's just my $0.02 worth.

As a separate idea from the above, I wonder if we could have gas
generate the DWARF required to represent the macro expansion as an
inlined function.  This might solve the problem of whether the DWARF
should point at the location of the macro definition, or the macro
invocation...

Anyway, hopefully the reproducer helps track down the problem.

Thanks,
Andrew
  

On 14.12.2022 17:58, Andrew Burgess wrote:
> I think I understand the issue a little more now.  I have a simple
> reproducer which can be run outside the gdb testsuite (see below).
> 
> It appears that the DWARF for macros now tries to associate the
> instructions within the macro the source location within the macro
> definition, rather than the macro use site.  I'm not entirely convinced
> this is a good idea (as a macro could be used multiple times), or even
> if this was an intended change of this series.

No, there was no intention to alter generated Dwarf. In fact I had put in
place a test ahead of this change here (commit 6fdb723799e2) to have at
least some proof of that. Quite likely that wasn't elaborate enough a test
then.

> If this is the direction gas is moving in then I guess we will need to
> update the GDB test, but there is, I think, a bug in the generated
> DWARF, in that it appears that the wrong file name is being used.

While I would like to improve representation of .macro expansions, we
first need to determine what the best way is for representing them. See
also the post-commit-message remark in the submission of the patch here.

Jan
  

On 14.12.2022 17:58, Andrew Burgess wrote:
> Anyway, hopefully the reproducer helps track down the problem.

It did indeed. Below the patch that addresses the issue for me,
but before committing I will want to run this through the full set
of tests (after extending the testcase which didn't catch the issue).

Jan

gas: restore Dwarf info generation after macro diagnostic adjustments

While 6fdb723799e2 ("gas: re-work line number tracking for macros and
their expansions") was meant to leave generated Dwarf as is, it really
didn't (and the testcase intended to catch that wasn't covering the case
which broke). Its adjustment to buffer_and_nest() didn't go far enough,
leading to the "linefile" directive inserted at the top to also be
processed later in the PR gas/16908 workaround (which clearly isn't
intended - it's being put there for processing during macro expansion
only). That unnoticed flaw in turn led me to worked around it by a
(suspicious to me already at the time) conditional in as_where().

--- a/gas/input-scrub.c
+++ b/gas/input-scrub.c
@@ -564,9 +564,6 @@ as_where (unsigned int *linep)
 
       do
 	{
-	  if (!saved->is_linefile)
-	    break;
-
 	  if (expansion != expanding_macro)
 	    /* Nothing.  */;
 	  else if (saved->logical_input_file != NULL
--- a/gas/macro.c
+++ b/gas/macro.c
@@ -120,8 +120,7 @@ buffer_and_nest (const char *from, const
   size_t from_len;
   size_t to_len = strlen (to);
   int depth = 1;
-  size_t line_start = ptr->len;
-  size_t more = get_line (ptr);
+  size_t line_start, more;
 
   if (to_len == 4 && strcasecmp (to, "ENDR") == 0)
     {
@@ -147,6 +146,8 @@ buffer_and_nest (const char *from, const
     xfree (linefile);
   }
 
+  line_start = ptr->len;
+  more = get_line (ptr);
   while (more)
     {
       /* Try to find the first pseudo op on the line.  */
  

Jan Beulich <jbeulich@suse.com> writes:

> On 14.12.2022 17:58, Andrew Burgess wrote:
>> Anyway, hopefully the reproducer helps track down the problem.
>
> It did indeed. Below the patch that addresses the issue for me,
> but before committing I will want to run this through the full set
> of tests (after extending the testcase which didn't catch the issue).
>
> Jan

Can confirm that the patch below fixes the gdb.asm/asm-source.exp test
failure that I was seeing.

Thanks,
Andrew

>
> gas: restore Dwarf info generation after macro diagnostic adjustments
>
> While 6fdb723799e2 ("gas: re-work line number tracking for macros and
> their expansions") was meant to leave generated Dwarf as is, it really
> didn't (and the testcase intended to catch that wasn't covering the case
> which broke). Its adjustment to buffer_and_nest() didn't go far enough,
> leading to the "linefile" directive inserted at the top to also be
> processed later in the PR gas/16908 workaround (which clearly isn't
> intended - it's being put there for processing during macro expansion
> only). That unnoticed flaw in turn led me to worked around it by a
> (suspicious to me already at the time) conditional in as_where().
>
> --- a/gas/input-scrub.c
> +++ b/gas/input-scrub.c
> @@ -564,9 +564,6 @@ as_where (unsigned int *linep)
>  
>        do
>  	{
> -	  if (!saved->is_linefile)
> -	    break;
> -
>  	  if (expansion != expanding_macro)
>  	    /* Nothing.  */;
>  	  else if (saved->logical_input_file != NULL
> --- a/gas/macro.c
> +++ b/gas/macro.c
> @@ -120,8 +120,7 @@ buffer_and_nest (const char *from, const
>    size_t from_len;
>    size_t to_len = strlen (to);
>    int depth = 1;
> -  size_t line_start = ptr->len;
> -  size_t more = get_line (ptr);
> +  size_t line_start, more;
>  
>    if (to_len == 4 && strcasecmp (to, "ENDR") == 0)
>      {
> @@ -147,6 +146,8 @@ buffer_and_nest (const char *from, const
>      xfree (linefile);
>    }
>  
> +  line_start = ptr->len;
> +  more = get_line (ptr);
>    while (more)
>      {
>        /* Try to find the first pseudo op on the line.  */