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

[Eric Christopher via Phabricator via llvm-commits]

echristo added inline comments.


================
Comment at: llvm/include/llvm/MC/MCELFStreamer.h:106
+                                    bool RelaxAll, bool IsThumb,
+                                    bool IsAndroid);
 
----------------
koutheir wrote:
> lattner wrote:
> > Can this subsume the isandroid" and potentially "is thumb" flags?
> I'm not sure. I didn't try to factor out things, in order to keep the changes minimal. A refactoring pass might choose to remove `IsThumb` or `IsAndroid`.
Could you do this? 


================
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),
----------------
Can you keep the same ordering as the construction function?


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


================
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; */) {
----------------
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?


================
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;
----------------
No commented out code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104759