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

[Renato Golin]

LGTM

On 19 November 2012 15:39, Amara Emerson <amara.emerson at arm.com> wrote:
> I've attached a new patch with some changes. Note that the patch is a git
> binary diff because of the new test object files, but I can supply separate
> patch + files if anyone wants me to.
>
> They are:
>  - Separate enum anonymous namespaces.
>  - Public getters/setters have been removed completely to make the class
> mostly just a data container.
>  - A new large test case added which exercises the previously non-tested
> attributes. llvm-readobj behaviour is congruent with readelf.
>
> Michael, does this look reasonable?
>
> Thanks,
> Amara
>
> -----Original Message-----
> From: rengolin at gmail.com [mailto:rengolin at gmail.com] On Behalf Of Renato
> Golin
> Sent: 16 November 2012 08:53
> To: Amara Emerson
> Cc: llvm-commits at cs.uiuc.edu for LLVM
> Subject: Re: [llvm-commits] [PATCH] ARM build attribute reading support
>
> On 15 November 2012 18:18, Amara Emerson <amara.emerson at arm.com> wrote:
>> Thanks for the comments. Yes, I realise the tests are a bit light at the
>> moment. I was hoping to get some general functionality comments as it
> would
>> probably take a few hundred lines to test each attribute type (and bloat
> the
>> llvm-readobj tool somewhat). If the general structure looks good I can add
>> more tests.
>
> Should be enough to have a few objects with different configurations
> for the attribute table on a test dir. And maybe one with a giant
> table with all possible arguments in it.
>
>
>>> Does big.LITTLE have its own value, here?
>>
>> No this is for the current ARM ELF ABI spec, which has a defined list of
>> architecture types.
>
> I know, but since big.LITTLE will certainly need new thoughts on how
> to optimise and especially how to select libraries (probably something
> like gnu's ifunc etc), would be good to have it in, if ARM already
> have something public towards that.
>
>
>> I don't have much of a preference really. The header file was an extended
>> version of the existing one in the ARM backend so I continued with the
> enum
>> style already used.
>
> I know, much of it was in the old file.
>
>
>> I used getters/setters because I wasn't sure on how to do error handling
> if
>> an invalid or strange attribute value was read. libObject seems different
> to
>> the rest of LLVM in this sense. E.g. should reading an invalid/malformed
>> attribute value result in an fatal error? If not, how should it warn or be
>> handled silently. If anyone who knows it better than me can give tips here
>> it'd be welcome.
>
> My personal feeling is that a malformed attribute table should be
> treated as an error, and you should not have setters in the class.
>
> So, instead of transforming that into a POD, I'd leave it all private,
> implement only the getters and let only that function create it
> (either by marking its class as friend or by having a way to construct
> it with all the information at once - maybe a factory).
>
> The point is that we may want to merge different tables, but we don't
> want to silently change any of the original tables, and each one must
> reflect exactly what was in the object file. The factory mentioned
> above could also hold the merging rules (when constructing from an
> array of tables, for instance), if that proves necessary in the
> future.
>
> --
> cheers,
> --renato
>
> http://systemcall.org/



-- 
cheers,
--renato

http://systemcall.org/