Page MenuHomePhabricator

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

Authored by dsanders on Jun 8 2016, 3:19 AM.

Details

Summary

The Mips implementation only covers the feature bits described by the ELF
e_flags so far. Mips stores additional feature bits such as MSA in the
.MIPS.abiflags section.

Also fixed a small bug this revealed where microMIPS wouldn't add the
EF_MIPS_MICROMIPS flag when using -filetype=obj.

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders updated this revision to Diff 60011.Jun 8 2016, 3:19 AM
dsanders retitled this revision from to [llvm-objdump] Support detection of feature bits from the object and implement this for Mips..
dsanders updated this object.
dsanders added a subscriber: llvm-commits.

Sounds reasonable, but why not return a SubtargetFeatures?
dsanders created this revision.
dsanders added a subscriber: llvm-commits.
Herald added subscribers: sdardis, dsanders.

The Mips implementation only covers the feature bits described by the ELF
e_flags so far. Mips stores additional feature bits such as MSA in the
.MIPS.abiflags section.

Also fixed a small bug this revealed where microMIPS wouldn't add the
EF_MIPS_MICROMIPS flag when using -filetype=obj.

http://reviews.llvm.org/D21125

Files:

include/llvm/Object/COFF.h
include/llvm/Object/ELFObjectFile.h
include/llvm/Object/MachO.h
include/llvm/Object/ObjectFile.h
include/llvm/Support/ELF.h
lib/Target/Mips/MipsAsmPrinter.cpp
test/CodeGen/Mips/Fast-ISel/shift.ll
test/CodeGen/Mips/compactbranches/no-beqzc-bnezc.ll
test/CodeGen/Mips/micromips-atomic1.ll
test/MC/Mips/cpload.s
test/MC/Mips/cprestore-noreorder-noat.s
test/MC/Mips/cprestore-noreorder.s
test/MC/Mips/cprestore-reorder.s
test/MC/Mips/cpsetup.s
test/MC/Mips/micromips-el-fixup-data.s
test/MC/Mips/mips64extins.s
test/MC/Mips/mips_gprel16.s
test/MC/Mips/set-defined-symbol.s
test/Object/Mips/feature.test
test/Object/Mips/objdump-micro-mips.test
tools/llvm-objdump/llvm-objdump.cpp

llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

dsanders updated this revision to Diff 60674.Jun 14 2016, 6:29 AM

Change getFeatureStr() to return a SubtargetFeatures object and rename it to
getFeatures().

rafael added inline comments.
include/llvm/Object/ELFObjectFile.h
949 ↗(On Diff #60674)

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.

tools/llvm-objdump/llvm-objdump.cpp
936 ↗(On Diff #60674)

Can you delete the MAttrs?

dsanders added inline comments.Jun 14 2016, 9:44 AM
include/llvm/Object/ELFObjectFile.h
949 ↗(On Diff #60674)

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.

tools/llvm-objdump/llvm-objdump.cpp
936 ↗(On Diff #60674)

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.

dsanders added inline comments.Jun 14 2016, 10:14 AM
include/llvm/Object/ELFObjectFile.h
949 ↗(On Diff #60674)

> 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?

rafael accepted this revision.Jun 15 2016, 6:46 AM
rafael added a reviewer: rafael.

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

include/llvm/Object/ELFObjectFile.h
949 ↗(On Diff #60674)

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.

This revision is now accepted and ready to land.Jun 15 2016, 6:46 AM
dsanders updated this revision to Diff 60846.Jun 15 2016, 9:04 AM
dsanders edited edge metadata.

Thanks.

Moved the implementation to ELFObjectFileBase and the .cpp. I'll commit in the morning.

I forgot to mention in the update message that there's also some small test changes to make it possible to commit this before the earlier patches in my current series.

dsanders closed this revision.Jun 16 2016, 2:23 AM
This revision was automatically updated to reflect the committed changes.