[PATCH] Record whether an object attribute is actually set

[PATCH] Record whether an object attribute is actually set

[Matthew Fortune]

Attached is a patch to explicitly record whether an ELF object
attribute has been set. This functionality will be used in the
MIPS port to determine the difference between a default value
and a user explicitly setting the default value.

OK to commit?

Tested with mips-linux-gnu and arm-linuxgnueabi as examples of
ports that use attributes albeit that they will not use the new
function. The feature has also been tested by using it in the
O32 FPXX patch.

Regards,
Matthew

2014-05-12  Matthew Fortune <matthew.fortune@imgtec.com>

bfd/
	* elf-attrs.c (bfd_elf_is_obj_attr_set): New function.
	(bfd_elf_add_obj_attr_int): Set is_set.
	(bfd_elf_add_obj_attr_string): Likewise.
	(bfd_elf_add_obj_attr_int_string): Likewise.
	(_bfd_elf_copy_obj_attributes): Likewise.
	* elf-bfd.h (obj_attribute): Add is_set field.
	(bfd_elf_is_obj_attr_set): New prototype.
---
 bfd/elf-attrs.c |   36 ++++++++++++++++++++++++++++++++++++
 bfd/elf-bfd.h   |    2 ++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
index cd0cbca..ad7d51f 100644
--- a/bfd/elf-attrs.c
+++ b/bfd/elf-attrs.c
@@ -263,6 +263,38 @@ elf_new_obj_attr (bfd *abfd, int vendor, int tag)
   return attr;
 }
 
+/* Determine if an attribute is set.  */
+bfd_boolean
+bfd_elf_is_obj_attr_set (bfd *abfd, int vendor, int tag)
+{
+  obj_attribute_list *p;
+  obj_attribute *attr = NULL;
+
+  if (tag < NUM_KNOWN_OBJ_ATTRIBUTES)
+    {
+      /* Known tags are preallocated.  */
+      attr = &elf_known_obj_attributes (abfd)[vendor][tag];
+    }
+  else
+    {
+      for (p = elf_other_obj_attributes (abfd)[vendor];
+	   p;
+	   p = p->next)
+	{
+	  if (tag == p->tag)
+	    {
+	      attr = &p->attr;
+	      break;
+	    }
+	  if (tag < p->tag)
+	    break;
+	}
+    }
+  return attr != NULL
+	 && (!is_default_attr (attr)
+	     || attr->is_set);
+}
+
 /* Return the value of an integer object attribute.  */
 int
 bfd_elf_get_obj_attr_int (bfd *abfd, int vendor, int tag)
@@ -298,6 +330,7 @@ bfd_elf_add_obj_attr_int (bfd *abfd, int vendor, int tag, unsigned int i)
   attr = elf_new_obj_attr (abfd, vendor, tag);
   attr->type = _bfd_elf_obj_attrs_arg_type (abfd, vendor, tag);
   attr->i = i;
+  attr->is_set = TRUE;
 }
 
 /* Duplicate an object attribute string value.  */
@@ -321,6 +354,7 @@ bfd_elf_add_obj_attr_string (bfd *abfd, int vendor, int tag, const char *s)
   attr = elf_new_obj_attr (abfd, vendor, tag);
   attr->type = _bfd_elf_obj_attrs_arg_type (abfd, vendor, tag);
   attr->s = _bfd_elf_attr_strdup (abfd, s);
+  attr->is_set = TRUE;
 }
 
 /* Add a int+string object attribute.  */
@@ -334,6 +368,7 @@ bfd_elf_add_obj_attr_int_string (bfd *abfd, int vendor, int tag,
   attr->type = _bfd_elf_obj_attrs_arg_type (abfd, vendor, tag);
   attr->i = i;
   attr->s = _bfd_elf_attr_strdup (abfd, s);
+  attr->is_set = TRUE;
 }
 
 /* Copy the object attributes from IBFD to OBFD.  */
@@ -360,6 +395,7 @@ _bfd_elf_copy_obj_attributes (bfd *ibfd, bfd *obfd)
 	{
 	  out_attr->type = in_attr->type;
 	  out_attr->i = in_attr->i;
+	  out_attr->is_set = TRUE;
 	  if (in_attr->s && *in_attr->s)
 	    out_attr->s = _bfd_elf_attr_strdup (obfd, in_attr->s);
 	  in_attr++;
diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 2c02135..3ec0ced 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -1472,6 +1472,7 @@ typedef struct obj_attribute
   int type;
   unsigned int i;
   char *s;
+  bfd_boolean is_set;
 } obj_attribute;
 
 typedef struct obj_attribute_list
@@ -2355,6 +2356,7 @@ extern void bfd_elf_add_obj_attr_int_string (bfd *, int, int, unsigned int,
 #define bfd_elf_add_proc_attr_int_string(BFD, TAG, INTVAL, STRVAL) \
   bfd_elf_add_obj_attr_int_string ((BFD), OBJ_ATTR_PROC, (TAG), \
 				   (INTVAL), (STRVAL))
+extern bfd_boolean bfd_elf_is_obj_attr_set (bfd *, int, int);
 
 extern char *_bfd_elf_attr_strdup (bfd *, const char *);
 extern void _bfd_elf_copy_obj_attributes (bfd *, bfd *);
-- 
1.7.1

Re: [PATCH] Record whether an object attribute is actually set

[Alan Modra]

On Mon, May 12, 2014 at 11:22:13PM +0000, Matthew Fortune wrote:
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -1472,6 +1472,7 @@ typedef struct obj_attribute
>    int type;
>    unsigned int i;
>    char *s;
> +  bfd_boolean is_set;
>  } obj_attribute;

Note that, because someone added a 2 * 71 array of this struct to
elf_obj_data, all ELF object BFDs get hit by any increase here,
whether the files use attributes or not.  Can you do without the flag,
somehow?

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Record whether an object attribute is actually set

[Richard Sandiford]

Sorry the slow reply to this.

Alan Modra <amodra@gmail.com> writes:
> On Mon, May 12, 2014 at 11:22:13PM +0000, Matthew Fortune wrote:
>> --- a/bfd/elf-bfd.h
>> +++ b/bfd/elf-bfd.h
>> @@ -1472,6 +1472,7 @@ typedef struct obj_attribute
>>    int type;
>>    unsigned int i;
>>    char *s;
>> +  bfd_boolean is_set;
>>  } obj_attribute;
>
> Note that, because someone added a 2 * 71 array of this struct to
> elf_obj_data, all ELF object BFDs get hit by any increase here,
> whether the files use attributes or not.  Can you do without the flag,
> somehow?

Yeah, I was more thinking about having a flag in the assembler,
since I think we want to know whether the attribute has been set
by an explicit .gnu_attribute in the code, rather than through
some implicit setting.

How about the attached look?

Thanks,
Richard


gas/
	* config/obj-elf.h (obj_elf_seen_attribute): Declare.
	* config/obj-elf.c (recorded_attribute_info): New structure.
	(recorded_attributes): New variable.
	(record_attribute, obj_elf_seen_attribute): New functions.
	(obj_elf_vendor_attribute): Record which attributes have been seen.

Index: gas/config/obj-elf.c
===================================================================
--- gas/config/obj-elf.c	2014-05-17 10:53:29.930903308 +0100
+++ gas/config/obj-elf.c	2014-05-17 11:04:20.847054304 +0100
@@ -1458,6 +1458,62 @@ skip_past_char (char ** str, char c)
 }
 #define skip_past_comma(str) skip_past_char (str, ',')
 
+/* A list of attributes that have been explicitly set by the assembly code.
+   VENDOR is the vendor id, BASE is the tag shifted right by the number
+   of bits in MASK, and bit N of MASK is set if tag BASE+N has been set.  */
+struct recorded_attribute_info {
+  struct recorded_attribute_info *next;
+  int vendor;
+  unsigned int base;
+  unsigned long mask;
+};
+static struct recorded_attribute_info *recorded_attributes;
+
+/* Record that we have seen an explicit specification of attribute TAG
+   for vendor VENDOR.  */
+
+static void
+record_attribute (int vendor, unsigned int tag)
+{
+  unsigned int base;
+  unsigned long mask;
+  struct recorded_attribute_info *rai;
+
+  base = tag / (8 * sizeof (rai->mask));
+  mask = 1UL << (tag % (8 * sizeof (rai->mask)));
+  for (rai = recorded_attributes; rai; rai = rai->next)
+    if (rai->vendor == vendor && rai->base == base)
+      {
+	rai->mask |= mask;
+	return;
+      }
+
+  rai = XNEW (struct recorded_attribute_info);
+  rai->next = recorded_attributes;
+  rai->vendor = vendor;
+  rai->base = base;
+  rai->mask = mask;
+  recorded_attributes = rai;
+}
+
+/* Return true if we have seen an explicit specification of attribute TAG
+   for vendor VENDOR.  */
+
+bfd_boolean
+obj_elf_seen_attribute (int vendor, unsigned int tag)
+{
+  unsigned int base;
+  unsigned long mask;
+  struct recorded_attribute_info *rai;
+
+  base = tag / (8 * sizeof (rai->mask));
+  mask = 1UL << (tag % (8 * sizeof (rai->mask)));
+  for (rai = recorded_attributes; rai; rai = rai->next)
+    if (rai->vendor == vendor && rai->base == base)
+      return (rai->mask & mask) != 0;
+  return FALSE;
+}
+
 /* Parse an attribute directive for VENDOR.
    Returns the attribute number read, or zero on error.  */
 
@@ -1540,6 +1596,7 @@ #define CONVERT_SYMBOLIC_ATTRIBUTE(a) -1
       s = demand_copy_C_string (&len);
     }
 
+  record_attribute (vendor, tag);
   switch (type & 3)
     {
     case 3:
Index: gas/config/obj-elf.h
===================================================================
--- gas/config/obj-elf.h	2014-05-17 10:53:29.942903421 +0100
+++ gas/config/obj-elf.h	2014-05-17 10:53:30.553909216 +0100
@@ -165,6 +165,8 @@ extern void obj_elf_text (int);
   (const char *, int, bfd_vma, int, const char *, int, int);
 extern struct fix *obj_elf_vtable_inherit (int);
 extern struct fix *obj_elf_vtable_entry (int);
+extern bfd_boolean obj_elf_seen_attribute
+  (int, unsigned int);
 extern int obj_elf_vendor_attribute (int);
 
 /* BFD wants to write the udata field, which is a no-no for the

Re: [PATCH] Record whether an object attribute is actually set

[Richard Sandiford]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Sorry the slow reply to this.
>
> Alan Modra <amodra@gmail.com> writes:
>> On Mon, May 12, 2014 at 11:22:13PM +0000, Matthew Fortune wrote:
>>> --- a/bfd/elf-bfd.h
>>> +++ b/bfd/elf-bfd.h
>>> @@ -1472,6 +1472,7 @@ typedef struct obj_attribute
>>>    int type;
>>>    unsigned int i;
>>>    char *s;
>>> +  bfd_boolean is_set;
>>>  } obj_attribute;
>>
>> Note that, because someone added a 2 * 71 array of this struct to
>> elf_obj_data, all ELF object BFDs get hit by any increase here,
>> whether the files use attributes or not.  Can you do without the flag,
>> somehow?
>
> Yeah, I was more thinking about having a flag in the assembler,
> since I think we want to know whether the attribute has been set
> by an explicit .gnu_attribute in the code, rather than through
> some implicit setting.
>
> How about the attached look?

Er, that's what happens you change your mind between "how does the
attached look" and "how about the attached" half-way through the sentence.

RE: [PATCH] Record whether an object attribute is actually set

[Matthew Fortune]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
> > Sorry the slow reply to this.
> >
> > Alan Modra <amodra@gmail.com> writes:
> >> On Mon, May 12, 2014 at 11:22:13PM +0000, Matthew Fortune wrote:
> >>> --- a/bfd/elf-bfd.h
> >>> +++ b/bfd/elf-bfd.h
> >>> @@ -1472,6 +1472,7 @@ typedef struct obj_attribute
> >>>    int type;
> >>>    unsigned int i;
> >>>    char *s;
> >>> +  bfd_boolean is_set;
> >>>  } obj_attribute;
> >>
> >> Note that, because someone added a 2 * 71 array of this struct to
> >> elf_obj_data, all ELF object BFDs get hit by any increase here,
> >> whether the files use attributes or not.  Can you do without the flag,
> >> somehow?
> >
> > Yeah, I was more thinking about having a flag in the assembler,
> > since I think we want to know whether the attribute has been set
> > by an explicit .gnu_attribute in the code, rather than through
> > some implicit setting.
> >
> > How about the attached look?
> 
> Er, that's what happens you change your mind between "how does the
> attached look" and "how about the attached" half-way through the sentence.

At least it didn't turn into complete gibberish which I'm sure I'll
manage one day!

Sorry for not getting back to you sooner, I wanted to run it with the FPXX
tests and had a half broken testsuite as I was working on it. This looks
good. I don't think there was much value in the end in the flag being
stored directly in the attributes as its meaning would have been fuzzy at
link time.

Thanks,
Matthew

Re: [PATCH] Record whether an object attribute is actually set

[Alan Modra]

On Sat, May 17, 2014 at 11:09:41AM +0100, Richard Sandiford wrote:
> 	* config/obj-elf.h (obj_elf_seen_attribute): Declare.
> 	* config/obj-elf.c (recorded_attribute_info): New structure.
> 	(recorded_attributes): New variable.
> 	(record_attribute, obj_elf_seen_attribute): New functions.
> 	(obj_elf_vendor_attribute): Record which attributes have been seen.

Excellent.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Record whether an object attribute is actually set

[Richard Earnshaw]

On 18/05/14 21:30, Matthew Fortune wrote:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>> Sorry the slow reply to this.
>>>
>>> Alan Modra <amodra@gmail.com> writes:
>>>> On Mon, May 12, 2014 at 11:22:13PM +0000, Matthew Fortune wrote:
>>>>> --- a/bfd/elf-bfd.h
>>>>> +++ b/bfd/elf-bfd.h
>>>>> @@ -1472,6 +1472,7 @@ typedef struct obj_attribute
>>>>>    int type;
>>>>>    unsigned int i;
>>>>>    char *s;
>>>>> +  bfd_boolean is_set;
>>>>>  } obj_attribute;
>>>>
>>>> Note that, because someone added a 2 * 71 array of this struct to
>>>> elf_obj_data, all ELF object BFDs get hit by any increase here,
>>>> whether the files use attributes or not.  Can you do without the flag,
>>>> somehow?
>>>
>>> Yeah, I was more thinking about having a flag in the assembler,
>>> since I think we want to know whether the attribute has been set
>>> by an explicit .gnu_attribute in the code, rather than through
>>> some implicit setting.
>>>
>>> How about the attached look?
>>
>> Er, that's what happens you change your mind between "how does the
>> attached look" and "how about the attached" half-way through the sentence.
> 
> At least it didn't turn into complete gibberish which I'm sure I'll
> manage one day!
> 
> Sorry for not getting back to you sooner, I wanted to run it with the FPXX
> tests and had a half broken testsuite as I was working on it. This looks
> good. I don't think there was much value in the end in the flag being
> stored directly in the attributes as its meaning would have been fuzzy at
> link time.
> 
> Thanks,
> Matthew
> 

[With apologies if you already know all of this].

Hmm, there's a "beware of dragons" warning here.

The attributes model, which IIRC is derived from the ARM ABI attributes
model, works on the principle that if an object file does not explicitly
specify a value for an attribute, then it takes its default value (for
numeric attributes that's normally 0).

So the important thing to remember when adding new attributes is that
they aren't really new: they've always existed, even before they were
specified -- but old object files have always set the value to default.

What does that mean?  Well the key point is that you have to pick your
default quite carefully, so that it means what that object probably
would have done anyway.  At other times, the default might have to mean
"I don't care" or "I won't say" and you then have to trust the object to
have got things right.

Beware of defining new attributes where the default is never useful,
though, so that new objects have to define it to some non-zero value.
They'll bloat object files if you have too many of them.  They'll also
make assembler files more tricky to write.

R.

RE: [PATCH] Record whether an object attribute is actually set

[Matthew Fortune]

Richard Earnshaw <rearnsha@arm.com> writes:
> On 18/05/14 21:30, Matthew Fortune wrote:
> > Richard Sandiford <rdsandiford@googlemail.com> writes:
> >> Richard Sandiford <rdsandiford@googlemail.com> writes:
> >>> Sorry the slow reply to this.
> >>>
> >>> Alan Modra <amodra@gmail.com> writes:
> >>>> On Mon, May 12, 2014 at 11:22:13PM +0000, Matthew Fortune wrote:
> >>>>> --- a/bfd/elf-bfd.h
> >>>>> +++ b/bfd/elf-bfd.h
> >>>>> @@ -1472,6 +1472,7 @@ typedef struct obj_attribute
> >>>>>    int type;
> >>>>>    unsigned int i;
> >>>>>    char *s;
> >>>>> +  bfd_boolean is_set;
> >>>>>  } obj_attribute;
> >>>>
> >>>> Note that, because someone added a 2 * 71 array of this struct to
> >>>> elf_obj_data, all ELF object BFDs get hit by any increase here,
> >>>> whether the files use attributes or not.  Can you do without the flag,
> >>>> somehow?
> >>>
> >>> Yeah, I was more thinking about having a flag in the assembler,
> >>> since I think we want to know whether the attribute has been set
> >>> by an explicit .gnu_attribute in the code, rather than through
> >>> some implicit setting.
> >>>
> >>> How about the attached look?
> >>
> >> Er, that's what happens you change your mind between "how does the
> >> attached look" and "how about the attached" half-way through the
> sentence.
> >
> > At least it didn't turn into complete gibberish which I'm sure I'll
> > manage one day!
> >
> > Sorry for not getting back to you sooner, I wanted to run it with the
> FPXX
> > tests and had a half broken testsuite as I was working on it. This looks
> > good. I don't think there was much value in the end in the flag being
> > stored directly in the attributes as its meaning would have been fuzzy at
> > link time.
> >
> > Thanks,
> > Matthew
> >
> 
> [With apologies if you already know all of this].
> 
> Hmm, there's a "beware of dragons" warning here.
> 
> The attributes model, which IIRC is derived from the ARM ABI attributes
> model, works on the principle that if an object file does not explicitly
> specify a value for an attribute, then it takes its default value (for
> numeric attributes that's normally 0).
> 
> So the important thing to remember when adding new attributes is that
> they aren't really new: they've always existed, even before they were
> specified -- but old object files have always set the value to default.
> 
> What does that mean?  Well the key point is that you have to pick your
> default quite carefully, so that it means what that object probably
> would have done anyway.  At other times, the default might have to mean
> "I don't care" or "I won't say" and you then have to trust the object to
> have got things right.
> 
> Beware of defining new attributes where the default is never useful,
> though, so that new objects have to define it to some non-zero value.
> They'll bloat object files if you have too many of them.  They'll also
> make assembler files more tricky to write.

Thanks Richard, I was aware of this but it is a useful reminder anyway.
The work I am doing currently is adding new values to an existing
attribute tag. The attribute being extended is the tag for floating-point
ABI and the new feature is that an ABI is automatically inferred based
on command line options. The reason for this patch is that I need to be
able to distinguish between an assembly input which explicitly defines
the default value (which has a useful meaning) and one which has no such
explicit directive. I only infer an ABI if the user did not explicitly
set one.

Regards,
Matthew