Indicate ABI in ARM ELF header flags

[koutheir]

Summary

Indicate ABI in ARM ELF header flags

[lattner]

I'm not a competent reviewer for this area anymore, a few driveby comments tho

llvm/include/llvm/MC/MCELFStreamer.h

106

Can this subsume the isandroid" and potentially "is thumb" flags?

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

1204

80 columns

[koutheir]

llvm/include/llvm/MC/MCELFStreamer.h

106

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.

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

1204

I executed git clang-format HEAD~1 before generating this review, so this is the LLVM style.

[echristo]

llvm/include/llvm/MC/MCELFStreamer.h

106

Could you do this?

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

438

Can you keep the same ordering as the construction function?

582

Document please.

1204

If it's beyond 80 columns it isn't :) Could you verify please?

1205

No commented out code.

[rengolin]

Adding folks working on Arm currently.

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

1200

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.

1203

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.

1205

Why leave that out in the first place?

1238

This comment is outdated, as above.

1570

This comment is outdated, as above.

[peter.smith]

Sorry I think that we shouldn't be setting these flags at all in relocatable objects.

If you look at the definition for these flags in https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#52elf-header they are defined for executables only for example:

Set in executable file headers (e_type = ET_EXEC or ET_DYN) to note that the executable file was built to conform to the hardware floating-point procedure-call standard.

Compatible with legacy (pre version 5) gcc use as EF_ARM_VFP_FLOAT.

A linker is expected to derive these values from Tag_ABI_VFP_args see https://github.com/ARM-software/abi-aa/blob/main/addenda32/addenda32.rst

Tag_ABI_VFP_args, (=28), uleb128
    0  The user intended FP parameter/result passing to conform to AAPCS, base variant
    1  The user intended FP parameter/result passing to conform to AAPCS, VFP variant
    2  The user intended FP parameter/result passing to conform to tool chain-specific
       conventions
    3  Code is compatible with both the base and VFP variants; the user did not permit
       non-variadic functions to pass FP parameters/results

You can see this by compiling an object with GCC, there is no ELF flag set for EF_ARM_ABI_FLOAT_HARD, the linker will add it in based on the build attributes.
See https://github.com/llvm/llvm-project/blob/main/lld/ELF/InputFiles.cpp#L699 in LLD for similar logic.

The BuildAttributes can be set directly by the user with .eabi_attribute directive. I believe the compiler should set them automatically and the assembler might be able to set them based on the target. I'd check there if there is a problem.

[koutheir]

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

438

I didn't change any ordering. Could you elaborate?

582

Sure.

1200

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.

1203

Same feeling here. Do you propose to factor out the code somewhere?

1204

I verified, and it's still over 80 columns. I'll manually change the code to avoid this.

1205

Because I couldn't propagate the information necessary for that test. I fixed it.

1238

Is MCELFStreamer not clearing the assembler's e_flags anymore?

Should I remove this comment?

[koutheir]

@peter.smith, thank you for the analysis! I'm consequently backing out of this patch.

[rengolin]

In D104759#2836223, @koutheir wrote:

@peter.smith, thank you for the analysis! I'm consequently backing out of this patch.

Thanks! Please select action "Abandon revision".

llvm/include/llvm/MC/MCELFStreamer.h

private:
 
   bool SeenIdent = false;
 
   /// BundleGroups - The stack of fragments holding the bundle-locked
   /// instructions.
   SmallVector<MCDataFragment *, 4> BundleGroups;
 };
 
-MCELFStreamer *createARMELFStreamer(MCContext &Context,
+MCELFStreamer *createARMELFStreamer(const Triple &T, MCContext &Context,
                                     std::unique_ptr<MCAsmBackend> TAB,
                                     std::unique_ptr<MCObjectWriter> OW,
                                     std::unique_ptr<MCCodeEmitter> Emitter,
-                                    bool RelaxAll, bool IsThumb, bool IsAndroid);
+                                    bool RelaxAll, bool IsThumb,
+                                    bool IsAndroid);
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCELFSTREAMER_H

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

 /// by MachO. Beware!
 class ARMELFStreamer : public MCELFStreamer {
 public:
   friend class ARMTargetELFStreamer;
 
   ARMELFStreamer(MCContext &Context, std::unique_ptr<MCAsmBackend> TAB,
                  std::unique_ptr<MCObjectWriter> OW,
                  std::unique_ptr<MCCodeEmitter> Emitter, bool IsThumb,
-                 bool IsAndroid)
+                 bool IsAndroid, const Triple &TargetTriple)
       : MCELFStreamer(Context, std::move(TAB), std::move(OW),
                       std::move(Emitter)),
-        IsThumb(IsThumb), IsAndroid(IsAndroid) {
+        IsThumb(IsThumb), IsAndroid(IsAndroid), TargetTriple(TargetTriple) {
     EHReset();
   }
 
   ~ARMELFStreamer() override = default;
 
   void finishImpl() override;
 
   // ARM exception handling directives
case MCAF_Code32:
       return; // Change to ARM mode
     case MCAF_Code64:
       return;
     case MCAF_SubsectionsViaSymbols:
       return;
     }
   }
 
+  unsigned computeELFHeaderFlags() const;
+
   /// If a label is defined before the .type directive sets the label's type
   /// then the label can't be recorded as thumb function when the label is
   /// defined. We override emitSymbolAttribute() which is called as part of the
   /// parsing of .type so that if the symbol has already been defined we can
   /// record the label as Thumb. FIXME: there is a corner case where the state
   /// is changed in between the label definition and the .type directive, this
   /// is not expected to occur in practice and handling it would require the
   /// backend to track IsThumb for every label.
void SwitchToEHSection(StringRef Prefix, unsigned Type, unsigned Flags,
                          SectionKind Kind, const MCSymbol &Fn);
   void SwitchToExTabSection(const MCSymbol &FnStart);
   void SwitchToExIdxSection(const MCSymbol &FnStart);
 
   void EmitFixup(const MCExpr *Expr, MCFixupKind Kind);
 
   bool IsThumb;
   bool IsAndroid;
+  Triple TargetTriple;
   int64_t MappingSymbolCounter = 0;
 
   DenseMap<const MCSection *, std::unique_ptr<ElfMappingSymbolInfo>>
       LastMappingSymbols;
 
   std::unique_ptr<ElfMappingSymbolInfo> LastEMSInfo;
 
   // ARM Exception Handling Frame Information

 void ARMELFStreamer::finishImpl() {
   MCTargetStreamer &TS = *getTargetStreamer();
   ARMTargetStreamer &ATS = static_cast<ARMTargetStreamer &>(TS);
   ATS.finishAttributeSection();
 
   MCELFStreamer::finishImpl();
 }
 
+unsigned ARMELFStreamer::computeELFHeaderFlags() const {
+  // FIXME: This should eventually end up somewhere else where more
+  // 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;
+
+  // See: ARMBaseTargetMachine::isTargetHardFloat().
+  if ((TargetTriple.isOSBinFormatMachO() && TargetTriple.getSubArch() == Triple::ARMSubArch_v7em) || TargetTriple
+                                                                                                         .isOSWindows() /* || TargetABI == ARMBaseTargetMachine::ARM_ABI_AAPCS16; */) {
+    e_flags |= ELF::EF_ARM_ABI_FLOAT_HARD;
+  } else {
+    switch (TargetTriple.getEnvironment()) {
+    case Triple::GNUEABIHF:
+    case Triple::MuslEABIHF:
+    case Triple::EABIHF:
+      e_flags |= ELF::EF_ARM_ABI_FLOAT_HARD;
+      break;
+
+    case Triple::GNUEABI:
+    case Triple::MuslEABI:
+    case Triple::EABI:
+      e_flags |= ELF::EF_ARM_ABI_FLOAT_SOFT;
+      break;
+
+    default:;
+    }
+  }
+  return e_flags;
+}
+
 void ARMELFStreamer::reset() {
   MCTargetStreamer &TS = *getTargetStreamer();
   ARMTargetStreamer &ATS = static_cast<ARMTargetStreamer &>(TS);
   ATS.reset();
   MappingSymbolCounter = 0;
   MCELFStreamer::reset();
   LastMappingSymbols.clear();
   LastEMSInfo.reset();
+
   // MCELFStreamer clear's the assembler's e_flags. However, for
   // arm we manually set the ABI version on streamer creation, so
   // do the same here
-  getAssembler().setELFHeaderEFlags(ELF::EF_ARM_EABI_VER5);
+  getAssembler().setELFHeaderEFlags(computeELFHeaderFlags());
 }
 
 inline void ARMELFStreamer::SwitchToEHSection(StringRef Prefix,
                                               unsigned Type,
                                               unsigned Flags,
                                               SectionKind Kind,
                                               const MCSymbol &Fn) {
   const MCSectionELF &FnSection =

 MCTargetStreamer *createARMObjectTargetStreamer(MCStreamer &S,
                                                 const MCSubtargetInfo &STI) {
   const Triple &TT = STI.getTargetTriple();
   if (TT.isOSBinFormatELF())
     return new ARMTargetELFStreamer(S);
   return new ARMTargetStreamer(S);
 }
 
-MCELFStreamer *createARMELFStreamer(MCContext &Context,
+MCELFStreamer *createARMELFStreamer(const Triple &TargetTriple,
+                                    MCContext &Context,
                                     std::unique_ptr<MCAsmBackend> TAB,
                                     std::unique_ptr<MCObjectWriter> OW,
                                     std::unique_ptr<MCCodeEmitter> Emitter,
                                     bool RelaxAll, bool IsThumb,
                                     bool IsAndroid) {
   ARMELFStreamer *S =
       new ARMELFStreamer(Context, std::move(TAB), std::move(OW),
-                         std::move(Emitter), IsThumb, IsAndroid);
+                         std::move(Emitter), IsThumb, IsAndroid, TargetTriple);
   // FIXME: This should eventually end up somewhere else where more
   // 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(ELF::EF_ARM_EABI_VER5);
+  S->getAssembler().setELFHeaderEFlags(S->computeELFHeaderFlags());
 
   if (RelaxAll)
     S->getAssembler().setRelaxAll(true);
   return S;
 }
 
 } // end namespace llvm

llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

 }
 
 static MCStreamer *createELFStreamer(const Triple &T, MCContext &Ctx,
                                      std::unique_ptr<MCAsmBackend> &&MAB,
                                      std::unique_ptr<MCObjectWriter> &&OW,
                                      std::unique_ptr<MCCodeEmitter> &&Emitter,
                                      bool RelaxAll) {
   return createARMELFStreamer(
-      Ctx, std::move(MAB), std::move(OW), std::move(Emitter), false,
+      T, Ctx, std::move(MAB), std::move(OW), std::move(Emitter), false,
       (T.getArch() == Triple::thumb || T.getArch() == Triple::thumbeb),
       T.isAndroid());
 }
 
 static MCStreamer *
 createARMMachOStreamer(MCContext &Ctx, std::unique_ptr<MCAsmBackend> &&MAB,
                        std::unique_ptr<MCObjectWriter> &&OW,
                        std::unique_ptr<MCCodeEmitter> &&Emitter, bool RelaxAll,

llvm/test/MC/ARM/elf-eflags-eabi.s

 @ RUN: llvm-mc %s -triple=armv7-linux-gnueabi -filetype=obj -o - | \
 @ RUN:    llvm-readobj -h - | FileCheck -check-prefix=OBJ %s
   .syntax unified
   .text
   .globl  barf
   .align  2
   .type barf,%function
 barf:                                   @ @barf
 @ %bb.0:                                @ %entry
         b foo
 
-@@@ make sure the EF_ARM_EABIMASK comes out OK
+@@@ make sure the EF_ARM_EABIMASK and EF_ARM_ABI_FLOAT_SOFT come out OK
 @OBJ: ElfHeader {
-@OBJ:   Flags [ (0x5000000)
+@OBJ:   Flags [ (0x5000200)
+@OBJ-NEXT:    0x200
+@OBJ-NEXT:    0x1000000
+@OBJ-NEXT:    0x4000000
+@OBJ-NEXT:  ]

llvm/test/MC/ARM/elf-eflags-eabihf.s

-@ RUN: llvm-mc %s -triple=armv7-linux-gnueabi -filetype=obj -o - | \
+@ RUN: llvm-mc %s -triple=armv7-linux-gnueabihf -filetype=obj -o - | \
 @ RUN:    llvm-readobj -h - | FileCheck -check-prefix=OBJ %s
   .syntax unified
   .text
   .globl  barf
   .align  2
   .type barf,%function
 barf:                                   @ @barf
 @ %bb.0:                                @ %entry
         b foo
 
-@@@ make sure the EF_ARM_EABIMASK comes out OK
+@@@ make sure the EF_ARM_EABIMASK and EF_ARM_ABI_FLOAT_HARD come out OK
 @OBJ: ElfHeader {
-@OBJ:   Flags [ (0x5000000)
+@OBJ:   Flags [ (0x5000400)
+@OBJ-NEXT:    0x400
+@OBJ-NEXT:    0x1000000
+@OBJ-NEXT:    0x4000000
+@OBJ-NEXT:  ]