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

[Koutheir Attouchi via Phabricator via llvm-commits]

koutheir marked 2 inline comments as done.
koutheir added inline comments.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:438
                  std::unique_ptr<MCCodeEmitter> Emitter, bool IsThumb,
-                 bool IsAndroid)
+                 bool IsAndroid, const Triple &TargetTriple)
       : MCELFStreamer(Context, std::move(TAB), std::move(OW),
----------------
echristo wrote:
> Can you keep the same ordering as the construction function?
I didn't change any ordering. Could you elaborate?


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:582
 
+  unsigned computeELFHeaderFlags() const;
+
----------------
echristo wrote:
> Document please.
Sure.


================
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;
----------------
rengolin wrote:
> 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.
I don't know which target triples use EABI version 5 and which ones use different ABIs. That is the reason I'm initializing the variable with `EF_ARM_EABI_VER5`.

The information I'm adding here is about the behavior of the ABI regarding floating point registers.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:1203
+
+  // See: ARMBaseTargetMachine::isTargetHardFloat().
+  if ((TargetTriple.isOSBinFormatMachO() && TargetTriple.getSubArch() == Triple::ARMSubArch_v7em) || TargetTriple
----------------
rengolin wrote:
> 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.
Same feeling here. Do you propose to factor out the code somewhere?


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:1204
+  // See: ARMBaseTargetMachine::isTargetHardFloat().
+  if ((TargetTriple.isOSBinFormatMachO() && TargetTriple.getSubArch() == Triple::ARMSubArch_v7em) || TargetTriple
+                                                                                                         .isOSWindows() /* || TargetABI == ARMBaseTargetMachine::ARM_ABI_AAPCS16; */) {
----------------
echristo wrote:
> koutheir wrote:
> > lattner wrote:
> > > 80 columns
> > I executed `git clang-format HEAD~1` before generating this review, so this is the LLVM style.
> If it's beyond 80 columns it isn't :) Could you verify please?
I verified, and it's still over 80 columns. I'll manually change the code to avoid this.


================
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;
----------------
rengolin wrote:
> echristo wrote:
> > No commented out code.
> Why leave that out in the first place?
Because I couldn't propagate the information necessary for that test. I fixed it.


================
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());
----------------
rengolin wrote:
> This comment is outdated, as above.
Is `MCELFStreamer` not clearing the assembler's `e_flags` anymore?

Should I remove this comment?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104759