Commit: Add support for a temporary input line pointer in gas/read.c

Message ID 8760lk9vki.fsf@redhat.com
State New
Headers show

Commit Message

Nick Clifton Jan. 12, 2017, 2:55 p.m.
Hi Guys,

  I am checking in the attached patch to fix a internal assembler when
  gas is generating STABS output and it encounters a parsing error.  The
  problem was that the stabs functions would call the parsers in
  gas/read.c with a temporary input line pointer, but without setting
  the limit for this pointer.  So if a parsing error occurred, the
  assembler might attempt to read beyond the end of the temporary buffer
  which causes all kinds of problems.

  The patch solves the problem by adding a new function to gas/read.c to
  set a temporary input line pointer, and update the buffer_limit
  variable at the same time.  In this way the functions in stabs.c can
  safely create and use their temporary input line buffers.

  There are probably other places in gas where a temporary input line
  pointer is used and these functions would be helpful, but I have not
  investigated this yet.

Cheers
  Nick

gas/ChangeLog
2017-01-12  Nick Clifton  <nickc@redhat.com>

	* read.c (temp_ilp): New function.  Installs a temporary input
	line pointer.
	(restore_ilp): New function.  Restores the original input line
	pointer.
	* read.h (temp_ilp): Prototype.
	(restore_ilp): Prototype.
	* stabs.c (dot_func_p): Use bfd_boolean type.
	(generate_asm_file): Use temp_ilp and restore_ilp.
	(stabs_generate_asm_lineno): Likewise.
	(stabs_generate_asm_endfunc): Likewise.

Comments

Alan Modra Jan. 13, 2017, 12:26 a.m. | #1
Hi Nick,
On Thu, Jan 12, 2017 at 02:55:41PM +0000, Nick Clifton wrote:
> +static char *saved_ilp = NULL;

> +static char *saved_limit;


Can I suggest instead that you declare a struct save_ilp, use one of
them as a local var, and pass that by reference to the save/restore
functions.  They could be made inline too.

> +  /* Prevent the assert in restore_ilp from triggering if

> +     the input_line_pointer has not yet been initialised.  */

> +  if (saved_ilp == NULL)

> +    saved_limit = saved_ilp = (char *) "";


Changing saved_ilp means it really isn't saved!

-- 
Alan Modra
Australia Development Lab, IBM
Nick Clifton Jan. 13, 2017, 9:58 a.m. | #2
Hi Alan,

>> +static char *saved_ilp = NULL;

>> +static char *saved_limit;

> 

> Can I suggest instead that you declare a struct save_ilp, use one of

> them as a local var, and pass that by reference to the save/restore

> functions.  They could be made inline too.


I could do this, but why ?  What do we gain.  It is not like these functions
need to be fast, or that they affect the performance of the assembler.


>> +  /* Prevent the assert in restore_ilp from triggering if

>> +     the input_line_pointer has not yet been initialised.  */

>> +  if (saved_ilp == NULL)

>> +    saved_limit = saved_ilp = (char *) "";

> 

> Changing saved_ilp means it really isn't saved!


True, but I just wanted to avoid having to create a third local variable
saying 'the saved_ilp has been initialised'.  It does not matter that we
"restore" the input_line_pointer to an empty buffer, because the only way
that this situation can arise is if the input_line_pointer has not yet
been used.

Cheers
  Nick
Alan Modra Jan. 13, 2017, 11:19 a.m. | #3
On Fri, Jan 13, 2017 at 09:58:03AM +0000, Nick Clifton wrote:
> Hi Alan,

> 

> >> +static char *saved_ilp = NULL;

> >> +static char *saved_limit;

> > 

> > Can I suggest instead that you declare a struct save_ilp, use one of

> > them as a local var, and pass that by reference to the save/restore

> > functions.  They could be made inline too.

> 

> I could do this, but why ?  What do we gain.  It is not like these functions

> need to be fast, or that they affect the performance of the assembler.


We gain:
a) Less global state.  In this case, possibly doesn't affect things
   like https://sourceware.org/ml/binutils/2015-06/msg00010.html, but
   global state without good reason ought to be avoided.  Yes, you
   can tell me that next time I add some. :)
b) Reentrancy.  Do we save input_line_pointer and set it to a string
   somewhere then do the same again in some function called?  Only
   matters of course if you want to write a bit of generally useful
   infrastructure that might be used is other places.
c) Local state means asserts and setting saved_ilp NULL after use is
   hardly necessary.

> >> +  /* Prevent the assert in restore_ilp from triggering if

> >> +     the input_line_pointer has not yet been initialised.  */

> >> +  if (saved_ilp == NULL)

> >> +    saved_limit = saved_ilp = (char *) "";

> > 

> > Changing saved_ilp means it really isn't saved!

> 

> True, but I just wanted to avoid having to create a third local variable

> saying 'the saved_ilp has been initialised'.  It does not matter that we

> "restore" the input_line_pointer to an empty buffer, because the only way

> that this situation can arise is if the input_line_pointer has not yet

> been used.


If I had added the assert and I found it triggered on testing due to
input_line_pointer validly being NULL, I'd decide that I didn't really
need the assert!

-- 
Alan Modra
Australia Development Lab, IBM
Nick Clifton Jan. 18, 2017, 5:05 p.m. | #4
Hi Alan,

>> I could do this, but why ?  What do we gain.  It is not like these functions

>> need to be fast, or that they affect the performance of the assembler.

> 

> We gain:

> a) Less global state.  In this case, possibly doesn't affect things

>    like https://sourceware.org/ml/binutils/2015-06/msg00010.html, but

>    global state without good reason ought to be avoided.  Yes, you

>    can tell me that next time I add some. :)

> b) Reentrancy.  Do we save input_line_pointer and set it to a string

>    somewhere then do the same again in some function called?  Only

>    matters of course if you want to write a bit of generally useful

>    infrastructure that might be used is other places.

> c) Local state means asserts and setting saved_ilp NULL after use is

>    hardly necessary.


Fair enough.  I'll put this as a back-burner project for now.  Or maybe a
Google Summer of Code opportunity ?

Cheers
  Nick

Patch

diff --git a/gas/read.c b/gas/read.c
index 3669b28..4ca8b01 100644
--- a/gas/read.c
+++ b/gas/read.c
@@ -6333,3 +6333,47 @@  find_end_of_line (char *s, int mri_string)
 {
   return _find_end_of_line (s, mri_string, 0, 0);
 }
+
+static char *saved_ilp = NULL;
+static char *saved_limit;
+
+/* Use BUF as a temporary input pointer for calling other functions in this
+   file.  BUF must be a C string, so that its end can be found by strlen.
+   Also sets the buffer_limit variable (local to this file) so that buffer
+   overruns should not occur.  Saves the current input line pointer so that
+   it can be restored by calling restore_ilp().
+
+   Does not support recursion.
+
+   FIXME: This function is currently only used by stabs.c but that
+   should be extended to other files in the gas source directory.  */
+
+void
+temp_ilp (char *buf)
+{
+  gas_assert (saved_ilp == NULL);
+  gas_assert (buf != NULL);
+
+  saved_ilp = input_line_pointer;
+  saved_limit = buffer_limit;
+  /* Prevent the assert in restore_ilp from triggering if
+     the input_line_pointer has not yet been initialised.  */
+  if (saved_ilp == NULL)
+    saved_limit = saved_ilp = (char *) "";
+
+  input_line_pointer = buf;
+  buffer_limit = buf + strlen (buf);
+}
+
+/* Restore a saved input line pointer.  */
+
+void
+restore_ilp (void)
+{
+  gas_assert (saved_ilp != NULL);
+
+  input_line_pointer = saved_ilp;
+  buffer_limit = saved_limit;
+
+  saved_ilp = NULL;
+}
diff --git a/gas/read.h b/gas/read.h
index f1ccf92..e83118f 100644
--- a/gas/read.h
+++ b/gas/read.h
@@ -213,3 +213,5 @@  extern void s_xstab (int what);
 extern void s_rva (int);
 extern void s_incbin (int);
 extern void s_weakref (int);
+extern void temp_ilp (char *);
+extern void restore_ilp (void);
diff --git a/gas/stabs.c b/gas/stabs.c
index 49fc09b..f9127f0 100644
--- a/gas/stabs.c
+++ b/gas/stabs.c
@@ -46,13 +46,13 @@  static void generate_asm_file (int, const char *);
 #define STAB_STRING_SECTION_NAME ".stabstr"
 #endif
 
-/* Non-zero if we're in the middle of a .func function, in which case
+/* True if we're in the middle of a .func function, in which case
    stabs_generate_asm_lineno emits function relative line number stabs.
    Otherwise it emits line number stabs with absolute addresses.  Note that
    both cases only apply to assembler code assembled with -gstabs.  */
-static int in_dot_func_p;
+static bfd_boolean in_dot_func_p = FALSE;
 
-/* Label at start of current function if in_dot_func_p != 0.  */
+/* Label at start of current function if in_dot_func_p != FALSE.  */
 static const char *current_function_label;
 
 /*
@@ -510,7 +510,6 @@  generate_asm_file (int type, const char *file)
 {
   static char *last_file;
   static int label_count;
-  char *hold;
   char sym[30];
   char *buf;
   const char *tmp = file;
@@ -525,8 +524,6 @@  generate_asm_file (int type, const char *file)
      generate a string and then parse it again.  That lets us use the
      existing stabs hook, which expect to see a string, rather than
      inventing new ones.  */
-  hold = input_line_pointer;
-
   sprintf (sym, "%sF%d", FAKE_LABEL_NAME, label_count);
   ++label_count;
 
@@ -556,8 +553,10 @@  generate_asm_file (int type, const char *file)
 
   sprintf (bufp, "\",%d,0,0,%s\n", type, sym);
 
-  input_line_pointer = buf;
+  temp_ilp (buf);
   s_stab ('s');
+  restore_ilp ();
+
   colon (sym);
 
   if (last_file != NULL)
@@ -565,8 +564,6 @@  generate_asm_file (int type, const char *file)
   last_file = xstrdup (file);
 
   free (buf);
-
-  input_line_pointer = hold;
 }
 
 /* Generate stabs debugging information for the current line.  This is
@@ -576,7 +573,6 @@  void
 stabs_generate_asm_lineno (void)
 {
   static int label_count;
-  char *hold;
   const char *file;
   unsigned int lineno;
   char *buf;
@@ -590,8 +586,6 @@  stabs_generate_asm_lineno (void)
      existing stabs hook, which expect to see a string, rather than
      inventing new ones.  */
 
-  hold = input_line_pointer;
-
   file = as_where (&lineno);
 
   /* Don't emit sequences of stabs for the same line.  */
@@ -638,11 +632,13 @@  stabs_generate_asm_lineno (void)
       buf = XNEWVEC (char, 100);
       sprintf (buf, "%d,0,%d,%s\n", N_SLINE, lineno, sym);
     }
-  input_line_pointer = buf;
+
+  temp_ilp (buf);
   s_stab ('n');
+  restore_ilp ();
+
   colon (sym);
 
-  input_line_pointer = hold;
   outputting_stabs_line_debug = 0;
   free (buf);
 }
@@ -653,29 +649,30 @@  stabs_generate_asm_lineno (void)
 void
 stabs_generate_asm_func (const char *funcname, const char *startlabname)
 {
-  static int void_emitted_p;
-  char *hold = input_line_pointer;
+  static bfd_boolean void_emitted_p = FALSE;
   char *buf;
   unsigned int lineno;
 
   if (! void_emitted_p)
     {
-      input_line_pointer = (char *) "\"void:t1=1\",128,0,0,0";
+      temp_ilp ((char *) "\"void:t1=1\",128,0,0,0");
       s_stab ('s');
-      void_emitted_p = 1;
+      restore_ilp ();
+      void_emitted_p = TRUE;
     }
 
   as_where (&lineno);
   if (asprintf (&buf, "\"%s:F1\",%d,0,%d,%s",
 		funcname, N_FUN, lineno + 1, startlabname) == -1)
     as_fatal ("%s", xstrerror (errno));
-  input_line_pointer = buf;
+
+  temp_ilp (buf);
   s_stab ('s');
+  restore_ilp ();
   free (buf);
 
-  input_line_pointer = hold;
   current_function_label = xstrdup (startlabname);
-  in_dot_func_p = 1;
+  in_dot_func_p = TRUE;
 }
 
 /* Emit a stab to record the end of a function.  */
@@ -685,7 +682,6 @@  stabs_generate_asm_endfunc (const char *funcname ATTRIBUTE_UNUSED,
 			    const char *startlabname)
 {
   static int label_count;
-  char *hold = input_line_pointer;
   char *buf;
   char sym[30];
 
@@ -695,11 +691,12 @@  stabs_generate_asm_endfunc (const char *funcname ATTRIBUTE_UNUSED,
 
   if (asprintf (&buf, "\"\",%d,0,0,%s-%s", N_FUN, sym, startlabname) == -1)
     as_fatal ("%s", xstrerror (errno));
-  input_line_pointer = buf;
+
+  temp_ilp (buf);
   s_stab ('s');
+  restore_ilp ();
   free (buf);
 
-  input_line_pointer = hold;
-  in_dot_func_p = 0;
+  in_dot_func_p = FALSE;
   current_function_label = NULL;
 }