[PATCH] tools: add support for decoding ARM attributes

[Amara Emerson]

  Hi Saleem,

  Thanks a lot for doing this, it's been on my backlog for a while! I've done a cursory review of this based on Renato's good review.

  Some general comments:
    - Section and symbol attributes are deprecated and optional, see section 2.2.4 on "Formal syntax..."
    - We should only try to parse attributes which originate from the public "aeabi" pseudo-vendor. See section 2.2.5 for more details.
    - Can you emit the raw value of the attribute in the output, e.g. using the syntax "Tag_CPU_arch: (=10) v7" or similar. This will allow text parsers to obtain the actual value of the attribute instead of just a string. Our own LLVM build attribute tests can then easily use this. It also has the side benefit of allowing you to be a bit more descriptive with some of the more complicated attribute values.

  Other comments are inline. You should also wait for Richard to review some of the more nuanced attribute descriptions.


================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:331
@@ +330,3 @@
+  static const char *Strings[] = {
+    "Tag_FP_arch", "Single-Precision", "Reserved", "Tag_FP_arch (deprecated)"
+  };
----------------
Renato Golin wrote:
> My copy of the ABI might be a bit old, but it says:
> 
> Tag_ABI_HardFP_use, (=27), uleb128
> 0 The user intended that VFP use should be implied by Tag_FP_arch
> 1 The user permitted this entity to use only SP VFP instructions
> 2 The user permitted this entity to use only DP VFP instructions
> 3 The user permitted this entity to use both SP and DP VFP instructions
> (Note: This is effectively an explicit version of the default encoded by 0)
> 
> Richard, did that change with newer releases of the document?
Yes, r2.09 changed this.

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:321
@@ +320,3 @@
+                           uint32_t &Offset) {
+  static const char *Strings[] = { "Unused", "Small", "int", "forced to int" };
+  uint64_t Value = ParseInteger(Data, Offset);
----------------
Renato Golin wrote:
> This is more like: { "Forbidden", "Pack", "Int32", "Visible Int32" }
Yep, although Rich should clarify.

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:341
@@ +340,3 @@
+  static const char *Strings[] = {
+    "AAPCS", "AAPCS VFP", "Custom", "Not Permitted"
+  };
----------------
Renato Golin wrote:
> Are you sure aout "Not Permitted" here? My copy of the ABI doesn't have this (may be a new thing on ARMv8)
Same here, probably changed in newer version. Although "not permitted" seems a little terse, but probably correct enough.

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:401
@@ +400,3 @@
+                             uint32_t &Offset) {
+  static const char *Strings[] = { "If Available", "Permitted" };
+  uint64_t Value = ParseInteger(Data, Offset);
----------------
Renato Golin wrote:
> Not sure about the "If Available". My copy says:
> 
> Tag_FP_HP_extension (=36), uleb128 (formerly Tag_VFP_HP_extension = 36)
> 0 The user did not permit this entity to use the VFPv3/Advanced SIMD optional
> half-precision extension
> 1 Use of the VFPv3/Advanced SIMD optional half-precision extension was permitted
"If Available" seems correct according to r2.09.

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:486
@@ +485,3 @@
+    if (!Handled) {
+      if (Tag < 32 || Tag % 2 == 0)
+        IntegerAttribute(ARMBuildAttrs::AttrType(Tag), Data, Offset);
----------------
Renato Golin wrote:
> This is odd... Looks like it's just a guess. I'd print a warning or even an error. Or just use the generic warning/error function I mentioned before.
This is covered in "2.2.6 Coding extensibility and compatibility" for gracefully handling unknown tags.

Saleem, looks OK to me, but I would like to see an error here if there is an unhandled tag < 32. Tags 1-32 should always be handled.


http://llvm-reviews.chandlerc.com/D2576