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

[Anton Korobeynikov]

Hold on, I'm going to review this patch

On Mon, Nov 19, 2012 at 7:56 PM, Renato Golin <rengolin at systemcall.org> wrote:
> 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/
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits



-- 
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University