[PATCH] D104759: Indicate ABI in ARM ELF header flags

[Renato Golin via Phabricator via llvm-commits]

rengolin added reviewers: kristof.beyls, peter.smith, rovka.
rengolin added a comment.

Adding folks working on Arm currently.



================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:1200
+  // intelligent flag decisions can be made. For now we are just maintaining
+  // the status quo for ARM and setting EF_ARM_EABI_VER5 as the default.
+  unsigned e_flags = ELF::EF_ARM_EABI_VER5;
----------------
I don't think this comment is relevant anymore. You're definitely NOT "just maintaining the status quo" here, you're using the target triple to find the right ELF headers.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:1203
+
+  // See: ARMBaseTargetMachine::isTargetHardFloat().
+  if ((TargetTriple.isOSBinFormatMachO() && TargetTriple.getSubArch() == Triple::ARMSubArch_v7em) || TargetTriple
----------------
I'm really uncomfortable with the duplication of the code from `ARMTargetMachine`, more so because it's slightly different now, and whoever update `isTargetHardFloat` will not see this one, potentially creating hard-to-detect object generation issues.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:1205
+  if ((TargetTriple.isOSBinFormatMachO() && TargetTriple.getSubArch() == Triple::ARMSubArch_v7em) || TargetTriple
+                                                                                                         .isOSWindows() /* || TargetABI == ARMBaseTargetMachine::ARM_ABI_AAPCS16; */) {
+    e_flags |= ELF::EF_ARM_ABI_FLOAT_HARD;
----------------
echristo wrote:
> No commented out code.
Why leave that out in the first place?


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:1238
   // arm we manually set the ABI version on streamer creation, so
   // do the same here
+  getAssembler().setELFHeaderEFlags(computeELFHeaderFlags());
----------------
This comment is outdated, as above.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:1570
   // intelligent flag decisions can be made. For now we are just maintaining
   // the status quo for ARM and setting EF_ARM_EABI_VER5 as the default.
+  S->getAssembler().setELFHeaderEFlags(S->computeELFHeaderFlags());
----------------
This comment is outdated, as above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104759/new/

https://reviews.llvm.org/D104759