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

[Luis Machado <luis.machado@arm.com>]

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;
>   }
>   \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);
>