[llvm-commits] [PATCH] ARM build attribute reading support

[Renato Golin]

Hi Michael,

Thanks for the extensive review. I'll refrain form commenting on most
issues, only the ones I have knowledge on.


On 19 November 2012 20:38, Michael Spencer <bigcheesegs at gmail.com> wrote:
>> +  if (getArch() != Triple::arm)
>> +    report_fatal_error("Cannot read ARM build attributes of a non-ARM file.");
>> +
>> +  if (!dot_arm_attributes_sec)
>> +    report_fatal_error("No ARM attribute section found");
>
> There needs to be a way to check if calling this function is ok
> without crashing.

I was under the impression that when this function was being called,
the compiler already identified this to be EABI compliant with an
attributes section, so this check was more redundant than anything
else.

The attributes section is not mandatory, but if there is, it should be
consistent. So, either the caller check for the section before calling
this function (and the assert is ok), or this function returns if
there is no such section (and the assert should be a return).


> Is ULEB byte oriented? Or does endianess matter?

As far as I know, ULEB has no endiannes. But I could be wrong.


> There's no such thing as bigendian ARM, right?

Wrong. There is, it's just not very common, especially recently,
especially Cortex A.


> I have no idea how ARM attributes are formatted so I'm not sure about
> that part. But overall this patch seems very fragile and verbose.

They're just a binary string following key/value pairs (where the
value can have different types like strings or ulebs).

The attribute table reader (the part I was more focused on reviewing)
is self-contained and, well, a binary reader. There isn't much to do.

A long while ago I have added some bits to it, to cater for our
particular needs, but also followed more or less what was already
there. Amara's patch is just following the same trend, but with a more
complete implementation, so it's natural that the number of
enums/functions grow a bit out of hand.

I'm wondering if that could have been done with table-gen, but to be
honest, I'm not a table-gen wizard, so I'll pass it to someone else
that might be. Without table-gen, every strategy will bring either
bloat to the code (although localized), or macro complexity (again,
localized). I'm really fine either way, even a mix of them two.

If someone is feeling adventurous and want to do a table-gen version,
I'm also fine with it.

--
cheers,
--renato

http://systemcall.org/