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

[Andrew Burgess <aburgess@redhat.com>]

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;
>>   }
>>   \f
>> @@ -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);
>> +}
>>   \f
>>   /* 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);
>>