Re: [PATCH] ld: fix ABSOLUTE for general expressions

[Alan Modra <amodra at gmail dot com>]

On Tue, Jul 19, 2016 at 12:05:08PM +0200, Tristan Gingold wrote:
> 
> > On 19 Jul 2016, at 10:39, Alan Modra <amodra@gmail.com> wrote:
> > 
> > On Tue, Jul 19, 2016 at 09:06:49AM +0200, Tristan Gingold wrote:
> >> 
> >>> On 19 Jul 2016, at 03:02, Alan Modra <amodra@gmail.com> wrote:
> >>> 
> >>> On Mon, Jul 18, 2016 at 05:34:06PM +0200, Tristan Gingold wrote:
> >>>> Hello,
> >>>> 
> >>>> it appears that ABSOLUTE(x) doesn't generate an absolute value when X is not a symbol, it looks like it works only for symbols.
> >>>> See the testcase for an example.
> >>> [snip]
> >>>> +SECTIONS
> >>>> +{
> >>>> +  .text 0x100 :
> >>>> +  {
> >>>> +    *(.text)
> >>>> +   _stack_start = ABSOLUTE(0x0800);
> >>>> +  }
> >>>> +}
> >>> 
> >>> I think what you're missing is that a plain number inside an output
> >>> section statement is section relative, if the number is used as an
> >>> address.
> >> 
> >> But, this is not what the doc says:
> >> 
> >> ABSOLUTE(exp)
> >> Return the absolute (non-relocatable, as opposed to non-negative) value of the expression exp. Primarily useful to assign an absolute value to a symbol within a section definition, where symbol values are normally section relative.
> >> 
> >> I think the doc is clear, and according to it:
> >> 
> >> SECTIONS
> >> {
> >> .text 0x100 :
> >> {
> >>   *(.text)
> >>  _stack_start = ABSOLUTE(0x0800);
> >> }
> >> }
> >> 
> >> should set _stack_start to the value 0x800 (and not 0x900).
> > 
> > So how would you expect the following to behave?
> > 
> > SECTIONS
> > {
> >  .text 0x100 :
> >  {
> >    *(.text)
> >    x = 0x800;
> >    y = absolute (x);
> >    z = absolute (0x800);
> >  }
> > }
> > 
> > I would have thought that x, y and z should have the same value in an
> > executable, and they do as shown by nm.
> > 
> > 0000000000000900 T x
> > 0000000000000900 A y
> > 0000000000000900 T z
> 
> I agree with that for x and y, but not for z.
> 
> > There *is* a bug in the section of z.  Somehow it manages to be
> > section relative!  Hmm, no doubt due to premature folding of the
> > expression, in exp_unop.
> 
> Yes, and that is what my patch changes.
> 
> >> I am not against allowing only symbols for operand of ABSOLUTE.  I think this would be much clearer and still backward compatible in practice.
> >> 
> >> The rule you gave 'a plain number inside an output section statement is section relative' is not well defined.
> >> For example, how to apply it to:
> >>  x = 0x80 + 4;
> >> I know how ld would understand it: this is section_vma + 0x84.  But this is not exactly your rule!
> > 
> > In this case we have a section relative 0x80 added to a section
> > relative 4, which by the odd rules regarding operations on two values
> > in the same section, comes to a section relative 0x84.
> 
> I fail to draw a conclusion from your mail.  Do you agree there is a bug ?
> 
> Do you think we need to fix it (by changing the code) or do you think we need to change to doc (and only allow ABSOLUTE on symbols) ?

I believe exp_unop should not be calling exp_fold_tree, which would
leave the "absolute" token in the tree.  Similarly for the other
functions creating expression trees.  Interestingly, this causes
FAIL: ld-elf/maxpage3b but on examination it looks like the test was
added without properly checking the result.  So this fixes a bug in
evaluation of CONSTANT, and it also happens to give you the value you
want for your _stack_start testcase.  I'm going to play with it a bit
before committing, and of course fix the testsuite failure.

diff --git a/ld/ldexp.c b/ld/ldexp.c
index 68c4bc5..e72c13f 100644
--- a/ld/ldexp.c
+++ b/ld/ldexp.c
@@ -1263,9 +1263,6 @@ exp_binop (int code, etree_type *lhs, etree_type *rhs)
   value.binary.lhs = lhs;
   value.binary.rhs = rhs;
   value.type.node_class = etree_binary;
-  exp_fold_tree_no_dot (&value);
-  if (expld.result.valid_p)
-    return exp_intop (expld.result.value);
 
   new_e = (etree_type *) stat_alloc (sizeof (new_e->binary));
   memcpy (new_e, &value, sizeof (new_e->binary));
@@ -1284,9 +1281,6 @@ exp_trinop (int code, etree_type *cond, etree_type *lhs, etree_type *rhs)
   value.trinary.cond = cond;
   value.trinary.rhs = rhs;
   value.type.node_class = etree_trinary;
-  exp_fold_tree_no_dot (&value);
-  if (expld.result.valid_p)
-    return exp_intop (expld.result.value);
 
   new_e = (etree_type *) stat_alloc (sizeof (new_e->trinary));
   memcpy (new_e, &value, sizeof (new_e->trinary));
@@ -1303,9 +1297,6 @@ exp_unop (int code, etree_type *child)
   value.unary.type.lineno = child->type.lineno;
   value.unary.child = child;
   value.unary.type.node_class = etree_unary;
-  exp_fold_tree_no_dot (&value);
-  if (expld.result.valid_p)
-    return exp_intop (expld.result.value);
 
   new_e = (etree_type *) stat_alloc (sizeof (new_e->unary));
   memcpy (new_e, &value, sizeof (new_e->unary));
@@ -1323,10 +1314,6 @@ exp_nameop (int code, const char *name)
   value.name.name = name;
   value.name.type.node_class = etree_name;
 
-  exp_fold_tree_no_dot (&value);
-  if (expld.result.valid_p)
-    return exp_intop (expld.result.value);
-
   new_e = (etree_type *) stat_alloc (sizeof (new_e->name));
   memcpy (new_e, &value, sizeof (new_e->name));
   return new_e;

-- 
Alan Modra
Australia Development Lab, IBM