[PATCH] gas: fix bogus error on .org involving expression

[PATCH] gas: fix bogus error on .org involving expression

[Jan Beulich]

For years I have been carrying this change, and it was long forgotten
what it was originally meant to deal with, hence I've never submitted
it. Until I came across an issue with Linux-kernel-like alternative
instruction patching, where the space needed to hold the replacement
instruction was allocated using .org. Things built fine for me (since
I had the patch in place), and things also built fine on 2.20. But
assemblers from at least 2.22 onwards produce an undefined symbol
warning for the "orig" label in the new test case, followed by an
error complaining that .org would be moving backwards (which is a
logical consequence of the undefined symbol getting replaced by plain
zero).

The cvt_frag_to_fill() change is to deal with targets setting
TC_FINALIZE_SYMS_BEFORE_SIZE_SEG to zero. Such targets skip the
report_op_error() invocation in resolve_symbol_value(), when called
from S_GET_VALUE() a few lines up from where the change is being made.
(This in turn leads to the elf/bad-group testcase failing, or more
generally .sleb128 / .uleb128 otherwise silently accepting expressions
which weren't actually resolved.) This change in turn depends on the
hackish change to resolve_symbol_value(). All of this feels generally
wrong, so quite likely this change is only papering over an issue
elsewhere, or similar checks may need adding in further places (which
I haven't managed to discover yet).
---
Otoh the asymmetry of the code being changed in resolve_symbol_value()
looks bogus as well. To just name a few aspects:
- at least for commutative operators it's not clear why seg_left and
  seg_right should be treated differently,
- <constant> - <symbol> doesn't really make sense to "live" in the
  symbol's section,
- yet more complex expressions don't really make sense to "live" in any
  "normal" section,
- at least all relational operators really have absolute results.

--- a/gas/config/tc-hppa.c
+++ b/gas/config/tc-hppa.c
@@ -1282,6 +1282,7 @@ get_expression (char *str)
   input_line_pointer = str;
   seg = expression (&the_insn.exp);
   if (!(seg == absolute_section
+	|| seg == expr_section
 	|| seg == undefined_section
 	|| SEG_NORMAL (seg)))
     {
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -10894,6 +10894,7 @@ i386_finalize_immediate (segT exp_seg AT
 	   && exp_seg != text_section
 	   && exp_seg != data_section
 	   && exp_seg != bss_section
+	   && exp_seg != expr_section
 	   && exp_seg != undefined_section
 	   && !bfd_is_com_section (exp_seg))
     {
@@ -11192,6 +11193,7 @@ i386_finalize_displacement (segT exp_seg
 	   && exp_seg != text_section
 	   && exp_seg != data_section
 	   && exp_seg != bss_section
+	   && exp_seg != expr_section
 	   && exp_seg != undefined_section
 	   && !bfd_is_com_section (exp_seg))
     {
--- a/gas/config/tc-mmix.c
+++ b/gas/config/tc-mmix.c
@@ -2025,7 +2025,7 @@ mmix_greg_internal (char *label)
       expP->X_op_symbol = NULL;
     }
 
-  if (section == undefined_section)
+  if (section == undefined_section || section == expr_section)
     {
       /* This is an error or a LOC with an expression involving
 	 forward references.  For the expression to be correctly
@@ -3967,7 +3967,7 @@ s_loc (int ignore ATTRIBUTE_UNUSED)
       return;
     }
 
-  if (section == undefined_section)
+  if (section == undefined_section || section == expr_section)
     {
       /* This is an error or a LOC with an expression involving
 	 forward references.  For the expression to be correctly
@@ -4058,7 +4058,9 @@ s_loc (int ignore ATTRIBUTE_UNUSED)
 
   /* If we can't deduce the section, it must be the current one.
      Below, we arrange to assert this.  */
-  if (section != now_seg && section != undefined_section)
+  if (section != now_seg
+      && section != undefined_section
+      && section != expr_section)
     {
       obj_elf_section_change_hook ();
       subseg_set (section, 0);
@@ -4082,7 +4084,7 @@ s_loc (int ignore ATTRIBUTE_UNUSED)
 	  sym = exp.X_add_symbol;
 	  off = exp.X_add_number;
 
-	  if (section == undefined_section)
+	  if (section == undefined_section || section == expr_section)
 	    {
 	      /* We need an expr_symbol when tracking sections.  In
 		 order to make this an expr_symbol with file and line
@@ -4096,7 +4098,7 @@ s_loc (int ignore ATTRIBUTE_UNUSED)
 
       /* Track the LOC's where we couldn't deduce the section: assert
 	 that we weren't supposed to change section.  */
-      if (section == undefined_section)
+      if (section == undefined_section || section == expr_section)
 	{
 	  struct loc_assert_s *next = loc_asserts;
 	  loc_asserts = XNEW (struct loc_assert_s);
--- a/gas/expr.c
+++ b/gas/expr.c
@@ -2048,7 +2048,11 @@ expr (int rankarg,		/* Larger # is highe
 		  retval = absolute_section;
 		  rightseg = absolute_section;
 		}
+	      else
+		retval = expr_section;
 	    }
+	  else
+	    retval = expr_section;
 	}
       else
 	{
@@ -2060,17 +2064,18 @@ expr (int rankarg,		/* Larger # is highe
 	  resultP->X_add_number = 0;
 	  resultP->X_unsigned = 1;
 	  resultP->X_extrabit = 0;
+	  retval = expr_section;
 	}
 
       if (retval != rightseg)
 	{
-	  if (retval == undefined_section)
+	  if (retval == expr_section)
 	    ;
-	  else if (rightseg == undefined_section)
+	  else if (rightseg == expr_section)
 	    retval = rightseg;
-	  else if (retval == expr_section)
+	  else if (retval == undefined_section)
 	    ;
-	  else if (rightseg == expr_section)
+	  else if (rightseg == undefined_section)
 	    retval = rightseg;
 	  else if (retval == reg_section)
 	    ;
--- a/gas/symbols.c
+++ b/gas/symbols.c
@@ -1660,7 +1660,11 @@ resolve_symbol_value (symbolS *symp)
 		final_seg = undefined_section;
 	      else if (seg_left == absolute_section)
 		final_seg = seg_right;
+#if defined (TC_FINALIZE_SYMS_BEFORE_SIZE_SEG) && !TC_FINALIZE_SYMS_BEFORE_SIZE_SEG
+	      else if (finalize_syms || seg_right == absolute_section)
+#else
 	      else
+#endif
 		final_seg = seg_left;
 	    }
 	  resolved = (symbol_resolved_p (add_symbol)
--- /dev/null
+++ b/gas/testsuite/gas/all/expr-org.d
@@ -0,0 +1,11 @@
+#objdump: -s -j .data -j .alt
+#name: .org with expression (with forward reference)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 [0f][0f]ffff[0f][0f] 01[0 .]*
+
+Contents of section \.alt:
+ 0000 00000000 [ .]*
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/all/expr-org.s
@@ -0,0 +1,10 @@
+	.data
+orig:
+	.byte	0
+	.org	orig + (alt_end - alt_begin), 0xff
+	.pushsection .alt, "a", %progbits
+alt_begin:
+	.long	0
+alt_end:
+	.popsection
+	.byte	1
--- a/gas/testsuite/gas/all/gas.exp
+++ b/gas/testsuite/gas/all/gas.exp
@@ -457,6 +457,11 @@ gas_test_error "weakref4.s" "" "is alrea
 run_dump_test string
 if [is_elf_format] {
     run_dump_test none
+
+# Targets enabling OBJ_COMPLEX_RELC don't seem to be compatible with this.
+    if { ![istarget "mep-*-*"] } {
+	run_dump_test expr-org
+    }
 }
 
 run_dump_test quoted-sym-names
--- a/gas/write.c
+++ b/gas/write.c
@@ -467,6 +467,9 @@ cvt_frag_to_fill (segT sec ATTRIBUTE_UNU
 			  _("leb128 operand is an undefined symbol: %s"),
 			  S_GET_NAME (fragP->fr_symbol));
 	  }
+	else if (S_GET_SEGMENT (fragP->fr_symbol) == expr_section)
+	  as_bad_where (fragP->fr_file, fragP->fr_line,
+			_("leb128 operand is an unresolved expression"));
 
 	size = output_leb128 (fragP->fr_literal + fragP->fr_fix, value,
 			      fragP->fr_subtype);

Re: [PATCH] gas: fix bogus error on .org involving expression

[Alan Modra]

This is just speculation but I wonder if we could just drop
expr_section, and use undefined_section in places where we currently
create expression symbols?  Conceptually, is there any real difference
between an expression that we can't resolve just yet, and the simplest
case of such an expression, an undefined symbol that is a forward
reference?

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] gas: fix bogus error on .org involving expression

[Jan Beulich]

On 28.06.2022 05:15, Alan Modra wrote:
> This is just speculation but I wonder if we could just drop
> expr_section, and use undefined_section in places where we currently
> create expression symbols?  Conceptually, is there any real difference
> between an expression that we can't resolve just yet, and the simplest
> case of such an expression, an undefined symbol that is a forward
> reference?

Hmm, that's an interesting thought. There are a couple of places where
symbols are explicitly put in expr_section (cgen.c, dwarf2dbg.c,
read.c, tc-avr.c). I also see a possible problem with expression
resolution: A result there may want expressing to live outside of any
"real" section while at the same time also not being undefined. (In
fact that's what part of the patch here arranges for, to make those
cases actually distinguishable.) But it may well be that this is a
theoretical concern only, while really we may have no code which would
actually care.

I can try to find time to actually experiment along these lines, but
some of the mentioned uses don't look very clear how to deal with.
Nor does e.g. do_org() permitting expr_section but forbidding
undefined_section.

Jan

Re: [PATCH] gas: fix bogus error on .org involving expression

[Jan Beulich]

On 28.06.2022 05:15, Alan Modra wrote:
> This is just speculation but I wonder if we could just drop
> expr_section, and use undefined_section in places where we currently
> create expression symbols?

I've tried this, but failed miserably already on very basic code. In
particular resolve_symbol_value()'s handling of O_constant involves
going from expr_section to absolute_section. I cannot replace
expr_section there by undefined_section (or else, found the hard way,
at least all implicitly external symbols end up local absolute), but
clearly I also cannot simply remove that construct.

There are further places where already when doing the basic mechanical
change it seemed wrong to do that kind of replacement.

>  Conceptually, is there any real difference
> between an expression that we can't resolve just yet, and the simplest
> case of such an expression, an undefined symbol that is a forward
> reference?

The above may have to do with (at least) implicitly external symbols
not really being recorded as forward references. The forward_ref field
really is used for equates only afaict, and I don't think this can be
easily changed.

Jan

[PATCH] gas: fix bogus error on .org involving expression

[Jan Beulich]

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

For years I have been carrying this change, and it was long forgotten
what it was originally meant to deal with, hence I've never submitted
it. Until I came across an issue with Linux kernel like alternative
instruction patching, where the space needed to hold the replacement
instruction was allocated using .org. Things built fine for me (since
I had the patch in place), and things also built fine on 2.20. But
assemblers from at least 2.22 onwards produce an undefined symbol
warning for the "orig" label in the new test case, followed by an
error complaining that .org would be moving backwards (which is a
logical consequence of the undefined symbol getting replaced by plain
zero).

gas/
2016-03-02  Jan Beulich  <jbeulich@suse.com>

	* expr.c (expr): Set retval to expr_section for expressions
	involving symbols, which cannot be resolved right away.

gas/testsuite/
2016-03-02  Jan Beulich  <jbeulich@suse.com>

	* expr-org.s, expr.org.d: New.
	* gas.exp: Run new test.

--- 2016-03-02/gas/expr.c
+++ 2016-03-02/gas/expr.c
@@ -1998,7 +1998,11 @@ expr (int rankarg,		/* Larger # is highe
 		  retval = absolute_section;
 		  rightseg = absolute_section;
 		}
+	      else
+		retval = expr_section;
 	    }
+	  else
+	    retval = expr_section;
 	}
       else
 	{
@@ -2010,17 +2014,18 @@ expr (int rankarg,		/* Larger # is highe
 	  resultP->X_add_number = 0;
 	  resultP->X_unsigned = 1;
 	  resultP->X_extrabit = 0;
+	  retval = expr_section;
 	}
 
       if (retval != rightseg)
 	{
-	  if (retval == undefined_section)
+	  if (retval == expr_section)
 	    ;
-	  else if (rightseg == undefined_section)
+	  else if (rightseg == expr_section)
 	    retval = rightseg;
-	  else if (retval == expr_section)
+	  else if (retval == undefined_section)
 	    ;
-	  else if (rightseg == expr_section)
+	  else if (rightseg == undefined_section)
 	    retval = rightseg;
 	  else if (retval == reg_section)
 	    ;
--- 2016-03-02/gas/testsuite/gas/all/expr-org.d
+++ 2016-03-02/gas/testsuite/gas/all/expr-org.d
@@ -0,0 +1,11 @@
+#objdump: -s -j .data -j .alt
+#name: .org with expression (with forward reference)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 [0f][0f]ffff[0f][0f] 01 [ .]*
+
+Contents of section \.alt:
+ 0000 00000000 [ .]*
+#pass
--- 2016-03-02/gas/testsuite/gas/all/expr-org.s
+++ 2016-03-02/gas/testsuite/gas/all/expr-org.s
@@ -0,0 +1,10 @@
+	.data
+orig:
+	.byte	0
+	.org	orig + (alt_end - alt_begin), 0xff
+	.pushsection .alt, "a", @progbits
+alt_begin:
+	.long	0
+alt_end:
+	.popsection
+	.byte	1
--- 2016-03-02/gas/testsuite/gas/all/gas.exp
+++ 2016-03-02/gas/testsuite/gas/all/gas.exp
@@ -371,6 +371,7 @@ if {  ([istarget "i*86-*-*pe*"] && ![ist
 if { ![istarget "bfin-*-*"] && ![istarget "nds32*-*-*"] } then {
     run_dump_test assign
 }
+run_dump_test expr-org
 run_dump_test sleb128
 run_dump_test sleb128-2
 run_dump_test sleb128-3




[-- Attachment #2: binutils-master-master-org-expression.patch --]
[-- Type: text/plain, Size: 3019 bytes --]

gas: fix bogus error on .org involving expression

For years I have been carrying this change, and it was long forgotten
what it was originally meant to deal with, hence I've never submitted
it. Until I came across an issue with Linux kernel like alternative
instruction patching, where the space needed to hold the replacement
instruction was allocated using .org. Things built fine for me (since
I had the patch in place), and things also built fine on 2.20. But
assemblers from at least 2.22 onwards produce an undefined symbol
warning for the "orig" label in the new test case, followed by an
error complaining that .org would be moving backwards (which is a
logical consequence of the undefined symbol getting replaced by plain
zero).

gas/
2016-03-02  Jan Beulich  <jbeulich@suse.com>

	* expr.c (expr): Set retval to expr_section for expressions
	involving symbols, which cannot be resolved right away.

gas/testsuite/
2016-03-02  Jan Beulich  <jbeulich@suse.com>

	* expr-org.s, expr.org.d: New.
	* gas.exp: Run new test.

--- 2016-03-02/gas/expr.c
+++ 2016-03-02/gas/expr.c
@@ -1998,7 +1998,11 @@ expr (int rankarg,		/* Larger # is highe
 		  retval = absolute_section;
 		  rightseg = absolute_section;
 		}
+	      else
+		retval = expr_section;
 	    }
+	  else
+	    retval = expr_section;
 	}
       else
 	{
@@ -2010,17 +2014,18 @@ expr (int rankarg,		/* Larger # is highe
 	  resultP->X_add_number = 0;
 	  resultP->X_unsigned = 1;
 	  resultP->X_extrabit = 0;
+	  retval = expr_section;
 	}
 
       if (retval != rightseg)
 	{
-	  if (retval == undefined_section)
+	  if (retval == expr_section)
 	    ;
-	  else if (rightseg == undefined_section)
+	  else if (rightseg == expr_section)
 	    retval = rightseg;
-	  else if (retval == expr_section)
+	  else if (retval == undefined_section)
 	    ;
-	  else if (rightseg == expr_section)
+	  else if (rightseg == undefined_section)
 	    retval = rightseg;
 	  else if (retval == reg_section)
 	    ;
--- 2016-03-02/gas/testsuite/gas/all/expr-org.d
+++ 2016-03-02/gas/testsuite/gas/all/expr-org.d
@@ -0,0 +1,11 @@
+#objdump: -s -j .data -j .alt
+#name: .org with expression (with forward reference)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 [0f][0f]ffff[0f][0f] 01 [ .]*
+
+Contents of section \.alt:
+ 0000 00000000 [ .]*
+#pass
--- 2016-03-02/gas/testsuite/gas/all/expr-org.s
+++ 2016-03-02/gas/testsuite/gas/all/expr-org.s
@@ -0,0 +1,10 @@
+	.data
+orig:
+	.byte	0
+	.org	orig + (alt_end - alt_begin), 0xff
+	.pushsection .alt, "a", @progbits
+alt_begin:
+	.long	0
+alt_end:
+	.popsection
+	.byte	1
--- 2016-03-02/gas/testsuite/gas/all/gas.exp
+++ 2016-03-02/gas/testsuite/gas/all/gas.exp
@@ -371,6 +371,7 @@ if {  ([istarget "i*86-*-*pe*"] && ![ist
 if { ![istarget "bfin-*-*"] && ![istarget "nds32*-*-*"] } then {
     run_dump_test assign
 }
+run_dump_test expr-org
 run_dump_test sleb128
 run_dump_test sleb128-2
 run_dump_test sleb128-3