public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Binutils <binutils@sourceware.org>
Cc: Nick Clifton <nickc@redhat.com>,
	"ramana.radhakrishnan@arm.com" <ramana.radhakrishnan@arm.com>,
	Richard Earnshaw <rearnsha@arm.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Andrew Waterman <andrew@sifive.com>,
	Jim Wilson <jim.wilson.gcc@gmail.com>,
	Nelson Chu <nelson@rivosinc.com>,
	Marcus Shawcroft <marcus.shawcroft@arm.com>,
	Alan Modra <amodra@gmail.com>,
	Peter Bergner <bergner@vnet.ibm.com>,
	Geoff Keating <geoffk@geoffk.org>
Subject: [PATCH 5/5] gas: re-work line number tracking for macros and their expansions
Date: Tue, 29 Nov 2022 11:44:51 +0100	[thread overview]
Message-ID: <1c75f3b4-14ac-fa39-fac8-2c98b4c4dbab@suse.com> (raw)
In-Reply-To: <9afdf9c8-323b-78c1-d75b-8964e00cdec5@suse.com>

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

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.
---
Omitting testsuite adjustments from the inline patch, for its sheer size;
please see the attachment for the full (compressed) patch.
---
There are 86 more Arm "[Bb]ad MVE ..." testsuite failures, which I'm not
(yet) taking care of here. If the concept is not objected to, I'd go and
alter them all in the same mechanical way as done here for other
testcase adjustments. (In cases of macros which are used exactly once in
a testcase, of which there are a fair number, dropping the use of .macro
may also be an option, just like already done in "Arm: avoid unhelpful
use of .macro in testsuite" for another testcase.)
---
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
@@ -436,6 +436,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) */
 
@@ -443,6 +447,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) */
 
@@ -452,6 +460,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);
@@ -486,7 +495,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);
 

[-- Attachment #2: binutils-master-macro-location.patch.bz2 --]
[-- Type: application/octet-stream, Size: 26229 bytes --]

      parent reply	other threads:[~2022-11-29 10:44 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-29 10:34 [PATCH 0/5] gas: diagnostics for macros and some tidying Jan Beulich
2022-11-29 10:36 ` [PATCH 1/5] gas: avoid inserting extra newline in buffer_and_nest() Jan Beulich
2022-11-29 10:36 ` [PATCH 2/5] gas: squash (some) .linefile from listings Jan Beulich
2022-11-29 10:37 ` [PATCH 3/5] gas: add Dwarf line number test for .macro expansions Jan Beulich
2022-11-29 10:38 ` [PATCH 4/5] Arm: avoid unhelpful use of .macro in testsuite Jan Beulich
2022-11-29 10:44 ` Jan Beulich [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1c75f3b4-14ac-fa39-fac8-2c98b4c4dbab@suse.com \
    --to=jbeulich@suse.com \
    --cc=amodra@gmail.com \
    --cc=andrew@sifive.com \
    --cc=bergner@vnet.ibm.com \
    --cc=binutils@sourceware.org \
    --cc=geoffk@geoffk.org \
    --cc=jim.wilson.gcc@gmail.com \
    --cc=marcus.shawcroft@arm.com \
    --cc=nelson@rivosinc.com \
    --cc=nickc@redhat.com \
    --cc=palmer@dabbelt.com \
    --cc=ramana.radhakrishnan@arm.com \
    --cc=rearnsha@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).