[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) {
----------------
dsanders wrote:
> 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.
> > At least implement it in ELFObjectFileBase so it is not templated.
> Ok.

I'm not sure it's possible to eliminate the templating. The outermost switch is based on e_machine which has different types depending on the endianness of the ELF. Have I missed something?


http://reviews.llvm.org/D21125