diff mbox series

[3/5] gas: partly restore how current_location() had worked

Message ID 34da7da0-487f-48c4-b5c9-26616593612e@suse.com
State New
Headers
Series gas: dot handling |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Jan Beulich Nov. 22, 2024, 12:43 p.m. UTC 
  Commit 4a826962e760 changed its behavior without saying why, and without
putting in place any testcase demonstrating the required behavior.
Firmly latch the current position unless deferred-evaluation mode is in
effect.

Comments

Maciej W. Rozycki Nov. 26, 2024, 2:44 a.m. UTC | #1 
On Fri, 22 Nov 2024, Jan Beulich wrote:

> Commit 4a826962e760 changed its behavior without saying why, and without
> putting in place any testcase demonstrating the required behavior.

 That comes from CVS days when change descriptions were not our policy (I 
wish the policy was strictly followed nowadays too, sigh).

 Please refer these discussions:

<https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007240320070.29495@tp.orcam.me.uk/>
<https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007241701380.29495@tp.orcam.me.uk/>
<https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007241721120.29495@tp.orcam.me.uk/>
<https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007242332460.29495@tp.orcam.me.uk/>
<https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007260105250.29495@tp.orcam.me.uk/>

for the rationale behind this change and the other three in the series.  
There is a test case for the whole series included in 4/4.  Arguably the 
example given in 2.5/4 should have been made a proper test case as well.  
Please feel free to use it now.

 Again, let me know if you have any further questions after reviewing the 
discussions and I'll try to address them (and again mind that it's been 14 
years since I worked on it and my memory may a bit hazy about it now).

 Thanks for cc-ing me on this thread or I could have easily missed it.

  Maciej
Jan Beulich Nov. 26, 2024, 8:25 a.m. UTC | #2 
On 26.11.2024 03:44, Maciej W. Rozycki wrote:
> On Fri, 22 Nov 2024, Jan Beulich wrote:
> 
>> Commit 4a826962e760 changed its behavior without saying why, and without
>> putting in place any testcase demonstrating the required behavior.
> 
>  That comes from CVS days when change descriptions were not our policy (I 
> wish the policy was strictly followed nowadays too, sigh).
> 
>  Please refer these discussions:
> 
> <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007240320070.29495@tp.orcam.me.uk/>
> <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007241701380.29495@tp.orcam.me.uk/>
> <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007241721120.29495@tp.orcam.me.uk/>
> <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007242332460.29495@tp.orcam.me.uk/>
> <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007260105250.29495@tp.orcam.me.uk/>
> 
> for the rationale behind this change and the other three in the series.  
> There is a test case for the whole series included in 4/4.  Arguably the 
> example given in 2.5/4 should have been made a proper test case as well.  
> Please feel free to use it now.

For the .eqv case current_location() keeps using dot_symbol, even though -
see the cover letter of the series - I'm unconvinced dot_symbol is being
maintained as it should be (or alternatively dot_frag / dot_value aren't).

To give some background: I first tried to actually get rid of dot_symbol
again, yet that didn't work out as easily as I had hoped.

Jan
Maciej W. Rozycki Dec. 4, 2024, 1:54 a.m. UTC | #3 
On Tue, 26 Nov 2024, Jan Beulich wrote:

> >  Please refer these discussions:
> > 
> > <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007240320070.29495@tp.orcam.me.uk/>
> > <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007241701380.29495@tp.orcam.me.uk/>
> > <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007241721120.29495@tp.orcam.me.uk/>
> > <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007242332460.29495@tp.orcam.me.uk/>
> > <https://inbox.sourceware.org/binutils/alpine.DEB.1.10.1007260105250.29495@tp.orcam.me.uk/>
> > 
> > for the rationale behind this change and the other three in the series.  
> > There is a test case for the whole series included in 4/4.  Arguably the 
> > example given in 2.5/4 should have been made a proper test case as well.  
> > Please feel free to use it now.
> 
> For the .eqv case current_location() keeps using dot_symbol, even though -
> see the cover letter of the series - I'm unconvinced dot_symbol is being
> maintained as it should be (or alternatively dot_frag / dot_value aren't).

 I agree, we're inconsistent here.  I do hope you've found the discussions 
referred useful.

> To give some background: I first tried to actually get rid of dot_symbol
> again, yet that didn't work out as easily as I had hoped.

 I think it's `dot_frag'/`dot_value' actually that should go as both data 
objects can be retrieved from `dot_symbol' while `dot_symbol' can also be 
used in other ways.

  Maciej
diff mbox series

Patch

--- a/gas/expr.c
+++ b/gas/expr.c
@@ -728,7 +728,7 @@  mri_char_constant (expressionS *expressi
    handles the magic symbol `.'.  */
 
 void
-current_location (expressionS *expressionp)
+current_location (expressionS *expressionp, enum expr_mode mode)
 {
   if (now_seg == absolute_section)
     {
@@ -738,7 +738,15 @@  current_location (expressionS *expressio
   else
     {
       expressionp->X_op = O_symbol;
-      expressionp->X_add_symbol = &dot_symbol;
+      if (mode != expr_defer)
+	{
+	  expressionp->X_add_symbol = symbol_temp_new_now ();
+#ifdef tc_new_dot_label
+	  tc_new_dot_label (expressionp->X_add_symbol);
+#endif
+	}
+      else
+	  expressionp->X_add_symbol = &dot_symbol;
       expressionp->X_add_number = 0;
     }
 }
@@ -1215,14 +1223,14 @@  operand (expressionS *expressionP, enum
       if (is_part_of_name (*input_line_pointer))
 	goto isname;
 
-      current_location (expressionP);
+      current_location (expressionP, mode);
       break;
 #endif
 
     case '.':
       if (!is_part_of_name (*input_line_pointer))
 	{
-	  current_location (expressionP);
+	  current_location (expressionP, mode);
 	  break;
 	}
       else if ((strncasecmp (input_line_pointer, "startof.", 8) == 0
@@ -1309,7 +1317,7 @@  operand (expressionS *expressionP, enum
       if (! flag_m68k_mri || is_part_of_name (*input_line_pointer))
 	goto de_fault;
 
-      current_location (expressionP);
+      current_location (expressionP, mode);
       break;
 #endif
 
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -168,7 +168,7 @@  static int i386_finalize_displacement (s
 static int i386_att_operand (char *);
 static int i386_intel_operand (char *, int);
 static int i386_intel_simplify (expressionS *);
-static int i386_intel_parse_name (const char *, expressionS *);
+static int i386_intel_parse_name (const char *, expressionS *, enum expr_mode);
 static const reg_entry *parse_register (const char *, char **);
 static const char *parse_insn (const char *, char *, enum parse_mode);
 static char *parse_operands (char *, const char *);
@@ -16644,7 +16644,10 @@  parse_register (const char *reg_string,
 }
 
 int
-i386_parse_name (char *name, expressionS *e, char *nextcharP)
+i386_parse_name (char *name,
+		 expressionS *e,
+		 enum expr_mode mode,
+		 char *nextcharP)
 {
   const reg_entry *r = NULL;
   char *end = input_line_pointer;
@@ -16669,7 +16672,7 @@  i386_parse_name (char *name, expressionS
     }
   input_line_pointer = end;
   *end = 0;
-  return intel_syntax ? i386_intel_parse_name (name, e) : 0;
+  return intel_syntax ? i386_intel_parse_name (name, e, mode) : 0;
 }
 
 void
--- a/gas/config/tc-i386.h
+++ b/gas/config/tc-i386.h
@@ -191,8 +191,8 @@  extern bool i386_check_label (void);
 extern int i386_unrecognized_line (int);
 #define tc_unrecognized_line i386_unrecognized_line
 
-extern int i386_parse_name (char *, expressionS *, char *);
-#define md_parse_name(s, e, m, c) i386_parse_name (s, e, c)
+extern int i386_parse_name (char *, expressionS *, enum expr_mode, char *);
+#define md_parse_name(s, e, m, c) i386_parse_name (s, e, m, c)
 
 extern operatorT i386_operator (const char *name, unsigned int operands, char *);
 #define md_operator i386_operator
--- a/gas/config/tc-i386-intel.c
+++ b/gas/config/tc-i386-intel.c
@@ -236,13 +236,15 @@  operatorT i386_operator (const char *nam
   return O_absent;
 }
 
-static int i386_intel_parse_name (const char *name, expressionS *e)
+static int i386_intel_parse_name (const char *name,
+				  expressionS *e,
+				  enum expr_mode mode)
 {
   unsigned int j;
 
   if (! strcmp (name, "$"))
     {
-      current_location (e);
+      current_location (e, mode);
       return 1;
     }
 
--- a/gas/config/tc-mmix.h
+++ b/gas/config/tc-mmix.h
@@ -62,7 +62,7 @@  extern int mmix_gnu_syntax;
  (! mmix_gnu_syntax						\
   && (name[0] == '@'						\
       ? (! is_part_of_name (name[1])				\
-	 && (current_location (exp), 1))			\
+	 && (current_location (exp, mode), 1))			\
       : ((name[0] == ':' || ISUPPER (name[0]))			\
 	 && mmix_parse_predefined_name (name, exp))))
 
--- a/gas/expr.h
+++ b/gas/expr.h
@@ -184,7 +184,7 @@  extern segT expr (int, expressionS *, en
 extern unsigned int get_single_number (void);
 extern symbolS *make_expr_symbol (const expressionS * expressionP);
 extern int expr_symbol_where (symbolS *, const char **, unsigned int *);
-extern void current_location (expressionS *);
+extern void current_location (expressionS *, enum expr_mode);
 extern symbolS *expr_build_uconstant (offsetT);
 extern symbolS *expr_build_dot (void);
 extern uint32_t generic_bignum_to_int32 (void);