[PATCH] tools: add support for decoding ARM attributes

[Renato Golin]

  Hi Saleem,

  Thanks for working on this, it'll help a lot on identifying bugs and creating tests.

  I have quite a lot of comments, most of them on the names of the values, but some more critical to the structure of the patch. I'd welcome more reviewers, especially Richard Barton, since he has a good knowledge in this area. Again, since this is readobj, I'm not sure my comments about the structure are pertinent, but I thought I'd rather ask than let it go.

  About the structure:
   * I don't like the idea of Strings[] being hard-coded in each method. It should be higher up whenever the build attributes are defined, on ARMBuildAttributes.h or something. However, there are too many different options and they can change depending on the option (permitted/not-permitted can swap, etc). So, this may be ok. I'd let others chime in.
   * Warnings should be emitted when unknown values are parsed, possibly even errors, each time you print "Unknown". The same for unknown tags. I've commented in-place about the tags, but the values can either be out-of-range integer or in-range, but invalid.
   * I'd remove the additional namespace and make the class name more accurate.
   * I'm not sure about the benefits of making ParseAttributeList() O(1), since this is just readobj. Others should chime in.

  cheers,
  --renato


================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:117
@@ +116,3 @@
+  case 'M': Profile = "Microcontroller"; break;
+  case 'S': Profile = "System"; break;
+  case '0': Profile = "None"; break;
----------------
This is more like A/R than System, and the ABI describes it as "classic programmer's model". Not sure what short description to use here... system may be ok, though. Richard?

================
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);
----------------
This is more like: { "Forbidden", "Pack", "Int32", "Visible Int32" }

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:331
@@ +330,3 @@
+  static const char *Strings[] = {
+    "Tag_FP_arch", "Single-Precision", "Reserved", "Tag_FP_arch (deprecated)"
+  };
----------------
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?

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:341
@@ +340,3 @@
+  static const char *Strings[] = {
+    "AAPCS", "AAPCS VFP", "Custom", "Not Permitted"
+  };
----------------
Are you sure aout "Not Permitted" here? My copy of the ABI doesn't have this (may be a new thing on ARMv8)

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:362
@@ +361,3 @@
+    "None", "Prefer Speed", "Aggressive Speed", "Prefer Size",
+    "Aggressive Size", "Good Debugging", "Best Debugging"
+  };
----------------
I'd replace "Prefer *" for just "*", and "Good Debugging" for just "Debugging".

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:374
@@ +373,3 @@
+    "None", "Prefer Speed", "Aggressive Speed", "Prefer Size",
+    "Aggressive Size", "Prefer Accuracy", "Aggressive Accuracy"
+  };
----------------
same here

================
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);
----------------
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

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:410
@@ +409,3 @@
+                                 uint32_t &Offset) {
+  static const char *Strings[] = { "Not Permitted", "IEEE-754", "Alternative" };
+  uint64_t Value = ParseInteger(Data, Offset);
----------------
Better call "Alternative" as "VFPv3"

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:428
@@ +427,3 @@
+  static const char *Strings[] = {
+    "If Available", "Not Permitted", "Permitted"
+  };
----------------
This is more like { "Thumb-v7R/M", "Not Permitted", "v7A" }

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:473
@@ +472,3 @@
+    uint64_t Tag = decodeULEB128(Data + Offset, &Length);
+    Offset = Offset + Length;
+
----------------
Offset += Length would be slight more clear, I think.

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:477
@@ +476,3 @@
+    for (unsigned AHI = 0, AHE = countof(DisplayRoutines);
+         AHI != AHE && !Handled; ++AHI) {
+      if (DisplayRoutines[AHI].Attribute == Tag) {
----------------
It would be uglier, but if you inserted a few empty items on the DisplayRoutines array, you could transform this into O(1).

If you had a generic warning function on all non existent tags, it'd even report automatically.

================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:486
@@ +485,3 @@
+    if (!Handled) {
+      if (Tag < 32 || Tag % 2 == 0)
+        IntegerAttribute(ARMBuildAttrs::AttrType(Tag), Data, Offset);
----------------
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.

================
Comment at: tools/llvm-readobj/ARMAttributeParser.h:19
@@ +18,3 @@
+
+namespace ARMBuildAttrs {
+class Parser {
----------------
Not sure namespace here would be the best way to deal with it. Since this parser is independent, I'd have called it something like ARMBuildAttrParser or something and have left the namespace alone.


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