This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Riscv ld-elf/stab failure, and fake label cleanup.

On Mon, 20 Nov 2017 14:41:04 PST (-0800), Jim Wilson wrote:

This is an attempt to fix a riscv ld-elf/stab testsuite failure.  This is
failing because the riscv port redefines FAKE_LABEL_NAME, and there turns out
to be a lot of hard coded assumptions about the value of FAKE_LABEL_NAME in
gas.
For the record (Jim knows this, he explained half of it to me :)), we want it this way because, due to all our linker relaxation, we frequently end up with FAKE_LABEL_NAME in object files. Users kept thinking the '\001' in their objdump output was a bug, so we changed it. 


The first part of this patch involves changing code to use FAKE_LABEL_NAME
consistently, and adding a FAKE_LABEL_CHAR for the special character used to
indicate a FAKE_LABEL_NAME.  I fixed everything I found in the main gas
sources, but did not change stuff in config, because it isn't broken if it
doesn't redefine FAKE_LABEL_CHAR, and I can't easily test it.

The second part is to stop accepting FAKE_LABEL_CHAR unless we are in a string.
I noticed that I could put a \001 char in a source file, and gas would accept
it, which is wrong.  I added a testcase for that which fails without the patch
and works with the patch.  There is a \001 character in the testcase, hopefully
that gets through email OK.  This only affects the stab support.

The third part is that I modified S_IS_LOCAL to check FAKE_LABEL_CHAR.  This
was working correctly only if FAKE_LABEL_NAME used the same special character
as DOLLAR_LABEL_CHAR.  This is something the riscv port got wrong, because it
was an implicit assumption in the code.  Now it is explicitly checking
FAKE_LABEL_CHAR if different from DOLLAR_LABEL_CHAR.

Tested with binutils/gas/ld builds and checks for riscv32-elf, riscv64-elf,
and x86_64-linux.  One less ld failure for the two riscv targets with the
patch, and no regressions for x86-64.

Comments?
Looks good to me, though I'm only really qualified to comment on the RISC-V stuff. 



	gas/
	* as.c: Include write.h.
	(common_emul_init): Use FAKE_LABEL_NAME.
	* ecoff.c (add_file, ecoff_directive_end, ecoff_directive_loc):
	Likewise.
	(ecoff_build_symbols): Use FAKE_LABEL_CHAR.
	* expr.c (get_symbol_name): Use FAKE_LABEL_CHAR.  Accept only if
	input_from_string is TRUE.
	* read.c (input_from_string): New.
	(read_symbol_name): Use FAKE_LABEL_CHAR.  Accept only if
	input_from_string is TRUE.
	(temp_ilp): Set input_from_string to TRUE.
	(restore_ilp): Set input_from_string to FALSE.
	* read.h (input_from_string): Declare.
	* symbols.c: Include write.h
	(S_IS_LOCAL): Check for FAKE_LABEL_CHAR.
	(symbol_relc_make_sym): Fix comment refering to default fake label
	string.
	* write.h (FAKE_LABEL_CHAR): New.
	* config/tc-riscv.h (FAKE_LABEL_CHAR): Define.
	* testsuite/gas/all/err-fakelabel.s: New.
---
 gas/as.c                              |  5 +++--
 gas/config/tc-riscv.h                 |  3 +++
 gas/ecoff.c                           | 10 +++++-----
 gas/expr.c                            |  7 ++++---
 gas/read.c                            |  9 ++++++---
 gas/read.h                            |  1 +
 gas/symbols.c                         |  6 +++++-
 gas/testsuite/gas/all/err-fakelabel.s |  3 +++
 gas/write.h                           |  6 ++++++
 9 files changed, 36 insertions(+), 14 deletions(-)
 create mode 100644 gas/testsuite/gas/all/err-fakelabel.s

diff --git a/gas/as.c b/gas/as.c
index 31e62805af..fbf3257fbb 100644
--- a/gas/as.c
+++ b/gas/as.c
@@ -39,6 +39,7 @@
 #include "dwarf2dbg.h"
 #include "dw2gencfi.h"
 #include "bfdver.h"
+#include "write.h"

 #ifdef HAVE_ITBL_CPU
 #include "itbl-ops.h"
@@ -199,10 +200,10 @@ common_emul_init (void)
   if (this_emulation->fake_label_name == 0)
     {
       if (this_emulation->leading_underscore)
-	this_emulation->fake_label_name = "L0\001";
+	this_emulation->fake_label_name = FAKE_LABEL_NAME;
       else
 	/* What other parameters should we test?  */
-	this_emulation->fake_label_name = ".L0\001";
+	this_emulation->fake_label_name = "." FAKE_LABEL_NAME;
     }
 }
 #endif
diff --git a/gas/config/tc-riscv.h b/gas/config/tc-riscv.h
index d79d30564c..04ef467b14 100644
--- a/gas/config/tc-riscv.h
+++ b/gas/config/tc-riscv.h
@@ -39,6 +39,9 @@ struct expressionS;
    sure FAKE_LABEL_NAME is printable.  It still must be distinct from any
    real label name.  So, append a space, which other labels can't contain.  */
 #define FAKE_LABEL_NAME ".L0 "
+/* Changing the special character in FAKE_LABEL_NAME requires changing
+   FAKE_LABEL_CHAR too.  */
+#define FAKE_LABEL_CHAR ' '

 #define md_relax_frag(segment, fragp, stretch) \
   riscv_relax_frag (segment, fragp, stretch)
diff --git a/gas/ecoff.c b/gas/ecoff.c
index 325c3de362..2ec4076407 100644
--- a/gas/ecoff.c
+++ b/gas/ecoff.c
@@ -2227,7 +2227,7 @@ add_file (const char *file_name, int indx ATTRIBUTE_UNUSED, int fake)
   if (stabs_seen)
     {
       (void) add_ecoff_symbol (file_name, st_Nil, sc_Nil,
-			       symbol_new ("L0\001", now_seg,
+			       symbol_new (FAKE_LABEL_NAME, now_seg,
 					   (valueT) frag_now_fix (),
 					   frag_now),
 			       (bfd_vma) 0, 0, ECOFF_MARK_STAB (N_SOL));
@@ -3020,7 +3020,7 @@ ecoff_directive_end (int ignore ATTRIBUTE_UNUSED)
     as_warn (_(".end directive names unknown symbol"));
   else
     (void) add_ecoff_symbol ((const char *) NULL, st_End, sc_Text,
-			     symbol_new ("L0\001", now_seg,
+			     symbol_new (FAKE_LABEL_NAME, now_seg,
 					 (valueT) frag_now_fix (),
 					 frag_now),
 			     (bfd_vma) 0, (symint_t) 0, (symint_t) 0);
@@ -3264,7 +3264,7 @@ ecoff_directive_loc (int ignore ATTRIBUTE_UNUSED)
   if (stabs_seen)
     {
       (void) add_ecoff_symbol ((char *) NULL, st_Label, sc_Text,
-			       symbol_new ("L0\001", now_seg,
+			       symbol_new (FAKE_LABEL_NAME, now_seg,
 					   (valueT) frag_now_fix (),
 					   frag_now),
 			       (bfd_vma) 0, 0, lineno);
@@ -4104,10 +4104,10 @@ ecoff_build_symbols (const struct ecoff_debug_swap *backend,
 		      /* If an st_end symbol has an associated gas
 		         symbol, then it is a local label created for
 		         a .bend or .end directive.  Stabs line
-		         numbers will have \001 in the names.  */
+		         numbers will have FAKE_LABEL_CHAR in the names.  */
 		      if (local
 			  && sym_ptr->ecoff_sym.asym.st != st_End
-			  && strchr (sym_ptr->name, '\001') == 0)
+			  && strchr (sym_ptr->name, FAKE_LABEL_CHAR) == 0)
 			sym_ptr->ecoff_sym.asym.iss =
 			  add_string (&fil_ptr->strings,
 				      fil_ptr->str_hash,
diff --git a/gas/expr.c b/gas/expr.c
index 6fc707b8a5..fffb7f16ff 100644
--- a/gas/expr.c
+++ b/gas/expr.c
@@ -2369,12 +2369,13 @@ get_symbol_name (char ** ilp_return)
   char c;

   * ilp_return = input_line_pointer;
-  /* We accept \001 in a name in case this is being called with a
+  /* We accept FAKE_LABEL_CHAR in a name in case this is being called with a
      constructed string.  */
-  if (is_name_beginner (c = *input_line_pointer++) || c == '\001')
+  if (is_name_beginner (c = *input_line_pointer++)
+      || (input_from_string && c == FAKE_LABEL_CHAR))
     {
       while (is_part_of_name (c = *input_line_pointer++)
-	     || c == '\001')
+	     || (input_from_string && c == FAKE_LABEL_CHAR))
 	;
       if (is_name_ender (c))
 	c = *input_line_pointer++;
diff --git a/gas/read.c b/gas/read.c
index afa1a168a6..6247b3ff58 100644
--- a/gas/read.c
+++ b/gas/read.c
@@ -62,6 +62,7 @@
 #endif

 char *input_line_pointer;	/*->next char of source file to parse.  */
+bfd_boolean input_from_string = FALSE;

 #if BITS_PER_CHAR != 8
 /*  The following table is indexed by[(char)] and will break if
@@ -1684,16 +1685,16 @@ read_symbol_name (void)
       if (mbstowcs (NULL, name, len) == (size_t) -1)
 	as_warn (_("symbol name not recognised in the current locale"));
     }
-  else if (is_name_beginner (c) || c == '\001')
+  else if (is_name_beginner (c) || (input_from_string && c == FAKE_LABEL_CHAR))
     {
       ptrdiff_t len;

       name = input_line_pointer - 1;

-      /* We accept \001 in a name in case this is
+      /* We accept FAKE_LABEL_CHAR in a name in case this is
 	 being called with a constructed string.  */
       while (is_part_of_name (c = *input_line_pointer++)
-	     || c == '\001')
+	     || (input_from_string && c == FAKE_LABEL_CHAR))
 	;

       len = (input_line_pointer - name) - 1;
@@ -6385,6 +6386,7 @@ temp_ilp (char *buf)

   input_line_pointer = buf;
   buffer_limit = buf + strlen (buf);
+  input_from_string = TRUE;
 }

 /* Restore a saved input line pointer.  */
@@ -6396,6 +6398,7 @@ restore_ilp (void)

   input_line_pointer = saved_ilp;
   buffer_limit = saved_limit;
+  input_from_string = FALSE;

   saved_ilp = NULL;
 }
diff --git a/gas/read.h b/gas/read.h
index f16455d428..5bc7475484 100644
--- a/gas/read.h
+++ b/gas/read.h
@@ -19,6 +19,7 @@
    02110-1301, USA.  */

 extern char *input_line_pointer;	/* -> char we are parsing now.  */
+extern bfd_boolean input_from_string;

 /* Define to make whitespace be allowed in many syntactically
    unnecessary places.  Normally undefined.  For compatibility with
diff --git a/gas/symbols.c b/gas/symbols.c
index 19a1fa5728..19b3dbc667 100644
--- a/gas/symbols.c
+++ b/gas/symbols.c
@@ -25,6 +25,7 @@
 #include "obstack.h"		/* For "symbols.h" */
 #include "subsegs.h"
 #include "struc-symbol.h"
+#include "write.h"

 /* This is non-zero if symbols are case sensitive, which is the
    default.  */
@@ -2161,6 +2162,9 @@ S_IS_LOCAL (symbolS *s)
 	  && ! S_IS_DEBUG (s)
 	  && (strchr (name, DOLLAR_LABEL_CHAR)
 	      || strchr (name, LOCAL_LABEL_CHAR)
+#if FAKE_LABEL_CHAR != DOLLAR_LABEL_CHAR
+	      || strchr (name, FAKE_LABEL_CHAR)
+#endif
 	      || TC_LABEL_IS_LOCAL (name)
 	      || (! flag_keep_locals
 		  && (bfd_is_local_label (stdoutput, s->bsym)
@@ -3087,7 +3091,7 @@ symbol_relc_make_sym (symbolS * sym)
       || S_GET_SEGMENT (sym) == absolute_section)
     return symbol_relc_make_expr (& sym->sy_value);

-  /* This may be a "fake symbol" L0\001, referring to ".".
+  /* This may be a "fake symbol", referring to ".".
      Write out a special null symbol to refer to this position.  */
   if (! strcmp (S_GET_NAME (sym), FAKE_LABEL_NAME))
     return xstrdup (".");
diff --git a/gas/testsuite/gas/all/err-fakelabel.s b/gas/testsuite/gas/all/err-fakelabel.s
new file mode 100644
index 0000000000..1e87f2ee2c
--- /dev/null
+++ b/gas/testsuite/gas/all/err-fakelabel.s
@@ -0,0 +1,3 @@
+;# Test that fake labels aren't accepted.
+;# { dg-do assemble }
+label:			;# { dg-error "Error: " }
diff --git a/gas/write.h b/gas/write.h
index a238320d4e..4157bc971c 100644
--- a/gas/write.h
+++ b/gas/write.h
@@ -27,6 +27,12 @@
 #define FAKE_LABEL_NAME "L0\001"
 #endif

+/* This is the special character used to indicate a fake symbol.  Must be
+   present in FAKE_LABEL_NAME.  */
+#ifndef FAKE_LABEL_CHAR
+#define FAKE_LABEL_CHAR '\001'
+#endif
+
 #include "bit_fix.h"

 /*
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]