[PATCH] D21125: [llvm-objdump] Support detection of feature bits from the object and implement this for Mips.

[Daniel Sanders via llvm-commits]

dsanders added inline comments.

================
Comment at: include/llvm/Object/ELFObjectFile.h:949
@@ -946,1 +948,3 @@
 template <class ELFT>
+SubtargetFeatures ELFObjectFile<ELFT>::getFeatures() const {
+  switch (EF.getHeader()->e_machine) {
----------------
rafael wrote:
> I just noticed that this is entirely implemented on top of getPlatformFlags.
> 
> This means that we could 
> * Move this out of lib/Object. It is only needed by code that does disassembly, no?
> * Make this a non virtual function.
> * At least implement it in ELFObjectFileBase so it is not templated.
> 
> Please implement one of these.
> I just noticed that this is entirely implemented on top of getPlatformFlags.

That's correct at the moment but the MIPS implementation should eventually read the .MIPS.abiflags section to detect MSA, DSP, etc. Do any other targets store feature flags outside of the ELF e_flags?

> Move this out of lib/Object. It is only needed by code that does disassembly, no?

Yes, it's just for disassembly. Where would be the best place to put it?

> Make this a non virtual function.

The main reason it's a virtual function is to allow any object format to initialize the SubtargetFeatures for the disassembler. I assume COFF and MachO have something equivalent to the ELF e_flags but I don't know those formats.

> At least implement it in ELFObjectFileBase so it is not templated.

Ok.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:936
@@ -935,4 +935,3 @@
   if (MAttrs.size()) {
-    SubtargetFeatures Features;
     for (unsigned i = 0; i != MAttrs.size(); ++i)
       Features.AddFeature(MAttrs[i]);
----------------
rafael wrote:
> Can you delete the MAttrs?
A small number of ARM, AArch64, and Hexagon tests are still using it but only two of them fail when I stop the option having an effect.

The ones that fail when I remove the option are:
    LLVM :: CodeGen/ARM/trap.ll
    LLVM :: MC/AArch64/optional-hash.s
    LLVM :: MC/Hexagon/v60-misc.s
    LLVM :: Object/Mips/feature.test
    LLVM :: Object/Mips/objdump-micro-mips.test
    LLVM :: tools/llvm-objdump/ARM/macho-mattr-arm.test
of which these two fail when the option exists but has no effect:
    LLVM :: CodeGen/ARM/trap.ll
    LLVM :: tools/llvm-objdump/ARM/macho-mattr-arm.test

I can update the ones that don't need it, but I'm not sure how to fix the two ARM tests.


http://reviews.llvm.org/D21125