[PATCH] tools: add support for decoding ARM attributes
Saleem Abdulrasool
compnerd at compnerd.org
Mon Jan 20 12:42:07 PST 2014
Amara,
I'm aware of the fact that section and symbol attributes are deprecated (in fact, Ive updated the ARMBuildAttrs.h to note this already :-)), but, I dont think that we should simply ignore their existence (particular since we could have foreign objects as inputs).
I do like your idea of printing the raw values which I consider an oversight on my part in making this output useful and will revise the patch to emit that.
Furthermore, you raise an excellent point on the parsing only the AEABI vendor section. We could end up mis-parsing a vendor section otherwise. I will adopt that behaviour as well.
Richard,
The display parsing is entirely in llvm-readobj (since that is not needed for the emission). The emission is entirely in the target backend where it belongs. I can look at duplication between the IAS and the MCAssembler layers and see if anything there can be refactored to reduce some duplication as a separate change. The constants reside entirely in Support, so they are shared across all of the consumers.
================
Comment at: test/tools/llvm-readobj/ARM/attributes.s:55
@@ +54,3 @@
+@ CHECK: Tag_CPU_name: CORTEX-A9
+@ CHECK: Tag_CPU_arch: v7
+@ CHECK: Tag_CPU_arch_profile: Application
----------------
Richard Barton wrote:
> In the ABI document these are represented with a preceeding "ARM"
Oh? I must've missed that bit in the spec. Ill adjust it.
================
Comment at: test/tools/llvm-readobj/ARM/attributes.s:72
@@ +71,3 @@
+@ CHECK: Tag_ABI_FP_user_exceptions: IEEE-754
+@ CHECK: Tag_ABI_FP_number_model: IEEE-754
+@ CHECK: Tag_ABI_align_needed: 8-byte alignment
----------------
Richard Barton wrote:
> Agree with Amara's comment here - It is vital to see the attribute value as well as the description.
>
> It would be even better to see the verbatim description from the ABI addenda here too.
I will be updating the patch to print out the raw values as well. I think that it is a good idea and a clear oversight of error handling on my part when implementing this.
================
Comment at: tools/llvm-readobj/ARMAttributeParser.h:19
@@ +18,3 @@
+
+namespace ARMBuildAttrs {
+class Parser {
----------------
Renato Golin wrote:
> 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.
Yes, I agree that this would not serve a purpose in the core (which was my original line of thinking) and so moving this out of the namespace and renaming the parser sounds like a good idea.
================
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;
----------------
Richard Barton wrote:
> Renato Golin wrote:
> > 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?
> Agreed - I think the strings from the ABI document would be the most suitable thing to have here. "System" is an extension on the ARMv6M architecture and is the 'S' part in v6S-M and does not mean the same thing as it does in v7-S unfortunately.
Sure, Ill yank the string from r2.09.
================
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);
----------------
Richard Barton wrote:
> Amara Emerson wrote:
> > Renato Golin wrote:
> > > This is more like: { "Forbidden", "Pack", "Int32", "Visible Int32" }
> > Yep, although Rich should clarify.
> I think it would be best for the ABI descriptions to be used here.
I think that the ABI has some rather verbose descriptions; a short description along with raw values would go a long way to clarify values I think.
Renato, your descriptions are more precise, so Im happy to use those instead.
================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:331
@@ +330,3 @@
+ static const char *Strings[] = {
+ "Tag_FP_arch", "Single-Precision", "Reserved", "Tag_FP_arch (deprecated)"
+ };
----------------
Richard Barton wrote:
> Amara Emerson wrote:
> > 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.
> The latest ABI documents are available online at http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ihi0045d/index.html
r2.09 indicates 2 as reserved rather than DP (which was there until r2.08).
================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:341
@@ +340,3 @@
+ static const char *Strings[] = {
+ "AAPCS", "AAPCS VFP", "Custom", "Not Permitted"
+ };
----------------
Amara Emerson wrote:
> 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.
Im happy to take an alternative description (I did have some trouble trying to shrink the rather verbose descriptions in the ABI specification).
================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:362
@@ +361,3 @@
+ "None", "Prefer Speed", "Aggressive Speed", "Prefer Size",
+ "Aggressive Size", "Good Debugging", "Best Debugging"
+ };
----------------
Renato Golin wrote:
> I'd replace "Prefer *" for just "*", and "Good Debugging" for just "Debugging".
WFM
================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:374
@@ +373,3 @@
+ "None", "Prefer Speed", "Aggressive Speed", "Prefer Size",
+ "Aggressive Size", "Prefer Accuracy", "Aggressive Accuracy"
+ };
----------------
Renato Golin wrote:
> same here
Okay.
================
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);
----------------
Renato Golin wrote:
> Better call "Alternative" as "VFPv3"
I like that idea, will change that.
================
Comment at: tools/llvm-readobj/ARMAttributeParser.cpp:486
@@ +485,3 @@
+ if (!Handled) {
+ if (Tag < 32 || Tag % 2 == 0)
+ IntegerAttribute(ARMBuildAttrs::AttrType(Tag), Data, Offset);
----------------
Amara Emerson wrote:
> 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.
Yes, Tags 1-32 should always be handled, so that sounds quite reasonable.
http://llvm-reviews.chandlerc.com/D2576
More information about the llvm-commits
mailing list