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

[Rafael Ávila de Espíndola via llvm-commits]

rafael accepted this revision.
rafael added a reviewer: rafael.
rafael added a comment.
This revision is now accepted and ready to land.

LGTM with the implementation moved to ELFObjectFileBase and to the .cpp file.


================
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:
> 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?
Can you add a virtual "uint16_t getEMachine();" to ELFObjectFileBase?

With that you should be able to move this too to ELFObjectFIleBase and implement it in the .cpp file.




http://reviews.llvm.org/D21125