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

[Amara Emerson]

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.

> Triple::thumb is not supported?

This is for checking the arch type of the ELF file, and the spec doesn't
differentiate between ARM/Thumb in that respect.

> 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'm in two minds about this. I know it doesn't matter much, but I
> think multiple anonymous enums would be clearer.

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.

> You seem to have many private fields, but public getters and setters,
> which is the same as POD struct. Why not use POD directly?

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.

Amara

-----Original Message-----
From: rengolin at gmail.com [mailto:rengolin at gmail.com] On Behalf Of Renato
Golin
Sent: 15 November 2012 17:59
To: Amara Emerson
Cc: llvm-commits at cs.uiuc.edu for LLVM
Subject: Re: [llvm-commits] [PATCH] ARM build attribute reading support

Hi Amara,

This is a great patch, but you need more tests, I believe. At least
one for each field you add, but possibly more.

I also have a few questions:

+  if (getArch() != Triple::arm)
+    report_fatal_error("Cannot read ARM build attributes of a non-ARM
file.");

Triple::thumb is not supported?

If so, shouldn't we check, or at least warn, for ISA compatibility?


+  // Legal Values for CPU_arch, (=6), uleb128
+  enum CPUArch {

Does big.LITTLE have its own value, here?


+  // The following have a lot of common use cases
+  enum {
+    //ARMISAUse (=8), uleb128  and THUMBISAUse (=9), uleb128
+    Not_Allowed = 0,
+    Allowed = 1,
+
+    // FP_arch (=10), uleb128 (formerly Tag_VFP_arch = 10)
+    AllowFPv2  = 2, // v2 FP ISA permitted (implies use of the v1 FP ISA)

(...)

I'm in two minds about this. I know it doesn't matter much, but I
think multiple anonymous enums would be clearer.


+  class ARMGenericBuildAttrInfo {
+public:
+    ARMGenericBuildAttrInfo()
+      : Tag_CPU_arch(v4T),
+        Tag_CPU_arch_profile(Not_Applicable),
(...)
+private:
+    CPUArch Tag_CPU_arch;
+    CPUArchProfile Tag_CPU_arch_profile;

You seem to have many private fields, but public getters and setters,
which is the same as POD struct. Why not use POD directly?


The rest looks good.

cheers,
--renato