[ELF] Make TrapInstr and Filler byte arrays. NFC.

[jrtc27]

Summary

The uint32_t type does not clearly convey that these fields are
interpreted in the target endianness. Converting them to byte arrays
should make this more obvious and less error-prone.

[jrtc27]

Wrap argument list for fill, and avoid any byte swapping when calculating NonZeroFiller

[ruiu]

ELF/Arch/ARM.cpp

64 ↗

(On Diff #172944)

I believe clang-format wouldn't add spaces after { and before }.

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

Do you need to use read32 if you only want to know whether a value is zero or not?

[jrtc27]

Remove inner spaces around initializer list braces.

[jrtc27]

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

We need to know whether all four elements are zero. We could check each element individually (either in a loop or unrolled), but this seemed nicer, and I'm assuming it'll be optimised to a single 32-bit load on architectures with unaligned loads.

[ruiu]

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

Ah, but this is not a performance critical path at all, so I'd just use read32le.

[jrtc27]

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

I don't particularly like unnecessarily slowing down the code for big-endian hosts, even if it isn't a critical path. The only advantage I see is that it shortens the code a tiny bit, but that's only because there is no read32ne wrapper for this like read32[bl]e... So I would advocate for keeping it as-is, but at the end of the day I'll change it if necessary.

[grimar]

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

Does not seem you need llvm::support::endian:: prefix here since you added the using?

(And I would also just use our write32 from Target.h here.

inline void write32(void *P, uint32_t V) {
  llvm::support::endian::write32(P, V, Config->Endianness);
}

This code is called once for the output section, so it's about .. 10-30 calls in average?
Does not seem worth to think about slow down here.)

[grimar]

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

I meant 'read32' of course:

inline uint32_t read32(const void *P) {
  return llvm::support::endian::read32(P, Config->Endianness);
}

[jrtc27]

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

Well, that then uses the *target* endianness instead, which doesn't seem to be an improvement over just using LE or BE unconditionally (though I guess the common case is target == host, where it wouldn't byte swap on either BE or LE...).

[grimar]

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

Improvement here is simplicity. There is no point to use LE or BE and even think about which endianness should be used here. Because for this case it is not important and for a new code reader to see read32 is much better than to see llvm::support::endian::read32<endianness::native>, the first one does not raise questions.

[ruiu]

LGTM with this change.

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

Yeah, read32 is just more concise than llvm::support::endian::read32<endianness::native>, that's why I like that.

[jrtc27]

Use target-dependent read32

[jrtc27]

ELF/OutputSections.cpp

242 ↗

(On Diff #172944)

Ok, I'll concede :)

[grimar]

LGTM, thanks!

[atanasyan]

Any plan on committing this change?

[jrtc27]

In D54207#1298509, @atanasyan wrote:

Any plan on committing this change?

If that's directed at me, I don't have commit rights.

[atanasyan]

In D54207#1298851, @jrtc27 wrote:

In D54207#1298509, @atanasyan wrote:

Any plan on committing this change?

If that's directed at me, I don't have commit rights.

I will commit your patch soon.

[mstorsjo]

This broke compilation with GCC 5.3 on Ubuntu 16.04, with the following error:

In file included from ../tools/lld/ELF/Thunks.cpp:30:0:  
../tools/lld/ELF/Target.h:125:49: error: array must be initialized with a brace-enclosed initializer  
   std::array<uint8_t, 4> TrapInstr = {0, 0, 0, 0};  
                                                 ^
../tools/lld/ELF/Target.h:125:49: error: too many initializers for ‘std::array<unsigned char, 4ul>’

[ruiu]

Hmm, what does that gcc error actually mean? It seems to me that its a false error.

[mstorsjo]

In D54207#1299504, @ruiu wrote:

Hmm, what does that gcc error actually mean? It seems to me that its a false error.

I guess it means that it is confused by the syntax for default initialization of a member variable, when used in combination with a braced initializer list like this.

[ruiu]

Because we always overwrites the default {0,0,0,0} with an actual trap instruction in the ctors of the derived classes, I believe we can just remove = {0, 0, 0, 0} to tame gcc.

[mstorsjo]

In D54207#1299513, @ruiu wrote:

Because we always overwrites the default {0,0,0,0} with an actual trap instruction in the ctors of the derived classes, I believe we can just remove = {0, 0, 0, 0} to tame gcc.

That does indeed seem to work, thanks!

[ruiu]

I'll commit that change. Thanks!

[ruiu]

You already did that. :) Thanks.

lld/trunk/ELF/Arch/ARM.cpp

ARM::ARM() {
   TlsGotRel = R_ARM_TLS_TPOFF32;
   TlsModuleIndexRel = R_ARM_TLS_DTPMOD32;
   TlsOffsetRel = R_ARM_TLS_DTPOFF32;
   GotBaseSymInGotPlt = false;
   GotEntrySize = 4;
   GotPltEntrySize = 4;
   PltEntrySize = 16;
   PltHeaderSize = 32;
-  TrapInstr = 0xd4d4d4d4;
+  TrapInstr = {0xd4, 0xd4, 0xd4, 0xd4};
   NeedsThunks = true;
 }
 
 uint32_t ARM::calcEFlags() const {
   // The ABIFloatType is used by loaders to detect the floating point calling
   // convention.
   uint32_t ABIFloatType = 0;
   if (Config->ARMVFPArgs == ARMVFPArgKind::Base ||
if (!llvm::isUInt<27>(Offset)) {
     // We cannot encode the Offset, use the long form.
     writePltHeaderLong(Buf);
     return;
   }
   write32le(Buf + 0, PltData[0]);
   write32le(Buf + 4, PltData[1] | ((Offset >> 20) & 0xff));
   write32le(Buf + 8, PltData[2] | ((Offset >> 12) & 0xff));
   write32le(Buf + 12, PltData[3] | (Offset & 0xfff));
-  write32le(Buf + 16, TrapInstr); // Pad to 32-byte boundary
-  write32le(Buf + 20, TrapInstr);
-  write32le(Buf + 24, TrapInstr);
-  write32le(Buf + 28, TrapInstr);
+  memcpy(Buf + 16, TrapInstr.data(), 4); // Pad to 32-byte boundary
+  memcpy(Buf + 20, TrapInstr.data(), 4);
+  memcpy(Buf + 24, TrapInstr.data(), 4);
+  memcpy(Buf + 28, TrapInstr.data(), 4);
 }
 
 void ARM::addPltHeaderSymbols(InputSection &IS) const {
   addSyntheticLocal("$a", STT_NOTYPE, 0, 0, IS);
   addSyntheticLocal("$d", STT_NOTYPE, 16, 0, IS);
 }
 
 // Long form PLT entries that do not have any restrictions on the displacement
void ARM::writePlt(uint8_t *Buf, uint64_t GotPltEntryAddr,
   if (!llvm::isUInt<27>(Offset)) {
     // We cannot encode the Offset, use the long form.
     writePltLong(Buf, GotPltEntryAddr, PltEntryAddr, Index, RelOff);
     return;
   }
   write32le(Buf + 0, PltData[0] | ((Offset >> 20) & 0xff));
   write32le(Buf + 4, PltData[1] | ((Offset >> 12) & 0xff));
   write32le(Buf + 8, PltData[2] | (Offset & 0xfff));
-  write32le(Buf + 12, TrapInstr); // Pad to 16-byte boundary
+  memcpy(Buf + 12, TrapInstr.data(), 4); // Pad to 16-byte boundary
 }
 
 void ARM::addPltSymbols(InputSection &IS, uint64_t Off) const {
   addSyntheticLocal("$a", STT_NOTYPE, Off, 0, IS);
   addSyntheticLocal("$d", STT_NOTYPE, Off + 12, 0, IS);
 }
 
 bool ARM::needsThunk(RelExpr Expr, RelType Type, const InputFile *File,

lld/trunk/ELF/Arch/Mips.cpp

template <class ELFT> MIPS<ELFT>::MIPS() {
   GotPltEntrySize = sizeof(typename ELFT::uint);
   GotBaseSymInGotPlt = false;
   PltEntrySize = 16;
   PltHeaderSize = 32;
   CopyRel = R_MIPS_COPY;
   NoneRel = R_MIPS_NONE;
   PltRel = R_MIPS_JUMP_SLOT;
   NeedsThunks = true;
-  TrapInstr = 0xefefefef;
+  TrapInstr = {0xef, 0xef, 0xef, 0xef};
 
   if (ELFT::Is64Bits) {
     RelativeRel = (R_MIPS_64 << 8) | R_MIPS_REL32;
     TlsGotRel = R_MIPS_TLS_TPREL64;
     TlsModuleIndexRel = R_MIPS_TLS_DTPMOD64;
     TlsOffsetRel = R_MIPS_TLS_DTPREL64;
   } else {
     RelativeRel = R_MIPS_REL32;

lld/trunk/ELF/Arch/PPC64.cpp

PPC64::PPC64() {
   //   It is normally desirable to put segments with different characteristics
   //   in separate 256 Mbyte portions of the address space, to give the
   //   operating system full paging flexibility in the 64-bit address space.
   //
   // And because the lowest non-zero 256M boundary is 0x10000000, PPC64 linkers
   // use 0x10000000 as the starting address.
   DefaultImageBase = 0x10000000;
 
-  TrapInstr =
-      (Config->IsLE == sys::IsLittleEndianHost) ? 0x7fe00008 : 0x0800e07f;
+  write32(TrapInstr.data(), 0x7fe00008);
 }
 
 static uint32_t getEFlags(InputFile *File) {
   if (Config->EKind == ELF64BEKind)
     return cast<ObjFile<ELF64BE>>(File)->getObj().getHeader()->e_flags;
   return cast<ObjFile<ELF64LE>>(File)->getObj().getHeader()->e_flags;
 }
 

lld/trunk/ELF/Arch/X86.cpp

X86::X86() {
   TlsGotRel = R_386_TLS_TPOFF;
   TlsModuleIndexRel = R_386_TLS_DTPMOD32;
   TlsOffsetRel = R_386_TLS_DTPOFF32;
   GotEntrySize = 4;
   GotPltEntrySize = 4;
   PltEntrySize = 16;
   PltHeaderSize = 16;
   TlsGdRelaxSkip = 2;
-  TrapInstr = 0xcccccccc; // 0xcc = INT3
+  TrapInstr = {0xcc, 0xcc, 0xcc, 0xcc}; // 0xcc = INT3
 
   // Align to the non-PAE large page size (known as a superpage or huge page).
   // FreeBSD automatically promotes large, superpage-aligned allocations.
   DefaultImageBase = 0x400000;
 }
 
 static bool hasBaseReg(uint8_t ModRM) { return (ModRM & 0xc7) != 0x5; }
 

lld/trunk/ELF/Arch/X86_64.cpp

template <class ELFT> X86_64<ELFT>::X86_64() {
   TlsGotRel = R_X86_64_TPOFF64;
   TlsModuleIndexRel = R_X86_64_DTPMOD64;
   TlsOffsetRel = R_X86_64_DTPOFF64;
   GotEntrySize = 8;
   GotPltEntrySize = 8;
   PltEntrySize = 16;
   PltHeaderSize = 16;
   TlsGdRelaxSkip = 2;
-  TrapInstr = 0xcccccccc; // 0xcc = INT3
+  TrapInstr = {0xcc, 0xcc, 0xcc, 0xcc}; // 0xcc = INT3
 
   // Align to the large page size (known as a superpage or huge page).
   // FreeBSD automatically promotes large, superpage-aligned allocations.
   DefaultImageBase = 0x200000;
 }
 
 template <class ELFT>
 RelExpr X86_64<ELFT>::getRelExpr(RelType Type, const Symbol &S,

lld/trunk/ELF/OutputSections.h

 
 #include "Config.h"
 #include "InputSection.h"
 #include "LinkerScript.h"
 #include "Relocations.h"
 #include "lld/Common/LLVM.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/ELF.h"
+#include <array>
 
 namespace lld {
 namespace elf {
 
 struct PhdrEntry;
 class Symbol;
 struct EhSectionPiece;
 class EhInputSection;
public:
   MemoryRegion *MemRegion = nullptr;
   MemoryRegion *LMARegion = nullptr;
   Expr AddrExpr;
   Expr AlignExpr;
   Expr LMAExpr;
   Expr SubalignExpr;
   std::vector<BaseCommand *> SectionCommands;
   std::vector<StringRef> Phdrs;
-  llvm::Optional<uint32_t> Filler;
+  llvm::Optional<std::array<uint8_t, 4>> Filler;
   ConstraintKind Constraint = ConstraintKind::NoConstraint;
   std::string Location;
   std::string MemoryRegionName;
   std::string LMARegionName;
   bool NonAlloc = false;
   bool Noload = false;
   bool ExpressionsUseSymbols = false;
   bool InOverlay = false;
 
   template <class ELFT> void finalize();
   template <class ELFT> void writeTo(uint8_t *Buf);
   template <class ELFT> void maybeCompress();
 
   void sort(llvm::function_ref<int(InputSectionBase *S)> Order);
   void sortInitFini();
   void sortCtorsDtors();
 
 private:
   // Used for implementation of --compress-debug-sections option.
   std::vector<uint8_t> ZDebugHeader;
   llvm::SmallVector<char, 1> CompressedData;
 
-  uint32_t getFiller();
+  std::array<uint8_t, 4> getFiller();
 };
 
 int getPriority(StringRef S);
 
 std::vector<InputSection *> getInputSections(OutputSection* OS);
 
 // All output sections that are handled by the linker specially are
 // globally accessible. Writer initializes them, so don't use them

lld/trunk/ELF/OutputSections.cpp

 #include "llvm/Support/Compression.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/SHA1.h"
 
 using namespace llvm;
 using namespace llvm::dwarf;
 using namespace llvm::object;
+using namespace llvm::support::endian;
 using namespace llvm::ELF;
 
 using namespace lld;
 using namespace lld::elf;
 
 uint8_t Out::First;
 PhdrEntry *Out::TlsPhdr;
 OutputSection *Out::ElfHeader;
void OutputSection::sort(llvm::function_ref<int(InputSectionBase *S)> Order) {
   assert(Live);
   for (BaseCommand *B : SectionCommands)
     if (auto *ISD = dyn_cast<InputSectionDescription>(B))
       sortByOrder(ISD->Sections, Order);
 }
 
 // Fill [Buf, Buf + Size) with Filler.
 // This is used for linker script "=fillexp" command.
-static void fill(uint8_t *Buf, size_t Size, uint32_t Filler) {
+static void fill(uint8_t *Buf, size_t Size,
+                 const std::array<uint8_t, 4> &Filler) {
   size_t I = 0;
   for (; I + 4 < Size; I += 4)
-    memcpy(Buf + I, &Filler, 4);
-  memcpy(Buf + I, &Filler, Size - I);
+    memcpy(Buf + I, Filler.data(), 4);
+  memcpy(Buf + I, Filler.data(), Size - I);
 }
 
 // Compress section contents if this section contains debug info.
 template <class ELFT> void OutputSection::maybeCompress() {
   typedef typename ELFT::Chdr Elf_Chdr;
 
   // Compress only DWARF debug sections.
   if (!Config->CompressDebugSections || (Flags & SHF_ALLOC) ||
if (!CompressedData.empty()) {
     memcpy(Buf, ZDebugHeader.data(), ZDebugHeader.size());
     memcpy(Buf + ZDebugHeader.size(), CompressedData.data(),
            CompressedData.size());
     return;
   }
 
   // Write leading padding.
   std::vector<InputSection *> Sections = getInputSections(this);
-  uint32_t Filler = getFiller();
-  if (Filler)
+  std::array<uint8_t, 4> Filler = getFiller();
+  bool NonZeroFiller = read32(Filler.data()) != 0;
+  if (NonZeroFiller)
     fill(Buf, Sections.empty() ? Size : Sections[0]->OutSecOff, Filler);
 
   parallelForEachN(0, Sections.size(), [&](size_t I) {
     InputSection *IS = Sections[I];
     IS->writeTo<ELFT>(Buf);
 
     // Fill gaps between sections.
-    if (Filler) {
+    if (NonZeroFiller) {
       uint8_t *Start = Buf + IS->OutSecOff + IS->getSize();
       uint8_t *End;
       if (I + 1 == Sections.size())
         End = Buf + Size;
       else
         End = Buf + Sections[I + 1]->OutSecOff;
       fill(Start, End - Start, Filler);
     }

 // because the compiler keeps the original initialization order in a
 // translation unit and we need to respect that.
 // For more detail, read the section of the GCC's manual about init_priority.
 void OutputSection::sortInitFini() {
   // Sort sections by priority.
   sort([](InputSectionBase *S) { return getPriority(S->Name); });
 }
 
-uint32_t OutputSection::getFiller() {
+std::array<uint8_t, 4> OutputSection::getFiller() {
   if (Filler)
     return *Filler;
   if (Flags & SHF_EXECINSTR)
     return Target->TrapInstr;
-  return 0;
+  return {0, 0, 0, 0};
 }
 
 template void OutputSection::writeHeaderTo<ELF32LE>(ELF32LE::Shdr *Shdr);
 template void OutputSection::writeHeaderTo<ELF32BE>(ELF32BE::Shdr *Shdr);
 template void OutputSection::writeHeaderTo<ELF64LE>(ELF64LE::Shdr *Shdr);
 template void OutputSection::writeHeaderTo<ELF64BE>(ELF64BE::Shdr *Shdr);
 
 template void OutputSection::writeTo<ELF32LE>(uint8_t *Buf);

lld/trunk/ELF/ScriptParser.cpp

private:
   void readSearchDir();
   void readSections();
   void readTarget();
   void readVersion();
   void readVersionScriptCommand();
 
   SymbolAssignment *readSymbolAssignment(StringRef Name);
   ByteCommand *readByteCommand(StringRef Tok);
-  uint32_t readFill();
-  uint32_t parseFill(StringRef Tok);
+  std::array<uint8_t, 4> readFill();
+  std::array<uint8_t, 4> parseFill(StringRef Tok);
   bool readSectionDirective(OutputSection *Cmd, StringRef Tok1, StringRef Tok2);
   void readSectionAddressType(OutputSection *Cmd);
   OutputSection *readOverlaySectionDescription();
   OutputSection *readOutputSectionDescription(StringRef OutSec);
   std::vector<BaseCommand *> readOverlay();
   std::vector<StringRef> readOutputSectionPhdrs();
   InputSectionDescription *readInputSectionDescription(StringRef Tok);
   StringMatcher readFilePatterns();
return [=] {
     return Script->getDot();
   };
 }
 
 // Reads a FILL(expr) command. We handle the FILL command as an
 // alias for =fillexp section attribute, which is different from
 // what GNU linkers do.
 // https://sourceware.org/binutils/docs/ld/Output-Section-Data.html
-uint32_t ScriptParser::readFill() {
+std::array<uint8_t, 4> ScriptParser::readFill() {
   expect("(");
-  uint32_t V = parseFill(next());
+  std::array<uint8_t, 4> V = parseFill(next());
   expect(")");
   return V;
 }
 
 // Tries to read the special directive for an output section definition which
 // can be one of following: "(NOLOAD)", "(COPY)", "(INFO)" or "(OVERLAY)".
 // Tok1 and Tok2 are next 2 tokens peeked. See comment for readSectionAddressType below.
 bool ScriptParser::readSectionDirective(OutputSection *Cmd, StringRef Tok1, StringRef Tok2) {

 
 // Parses a given string as a octal/decimal/hexadecimal number and
 // returns it as a big-endian number. Used for `=<fillexp>`.
 // https://sourceware.org/binutils/docs/ld/Output-Section-Fill.html
 //
 // When reading a hexstring, ld.bfd handles it as a blob of arbitrary
 // size, while ld.gold always handles it as a 32-bit big-endian number.
 // We are compatible with ld.gold because it's easier to implement.
-uint32_t ScriptParser::parseFill(StringRef Tok) {
+std::array<uint8_t, 4> ScriptParser::parseFill(StringRef Tok) {
   uint32_t V = 0;
   if (!to_integer(Tok, V))
     setError("invalid filler expression: " + Tok);
 
-  uint32_t Buf;
-  write32be(&Buf, V);
+  std::array<uint8_t, 4> Buf;
+  write32be(Buf.data(), V);
   return Buf;
 }
 
 SymbolAssignment *ScriptParser::readProvideHidden(bool Provide, bool Hidden) {
   expect("(");
   SymbolAssignment *Cmd = readSymbolAssignment(next());
   Cmd->Provide = Provide;
   Cmd->Hidden = Hidden;

lld/trunk/ELF/Target.h

 //===- Target.h -------------------------------------------------*- C++ -*-===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_ELF_TARGET_H
 #define LLD_ELF_TARGET_H
 
 #include "InputSection.h"
 #include "lld/Common/ErrorHandler.h"
 #include "llvm/Object/ELF.h"
 #include "llvm/Support/MathExtras.h"
+#include <array>
 
 namespace lld {
 std::string toString(elf::RelType Type);
 
 namespace elf {
 class Defined;
 class InputFile;
 class Symbol;
public:
 
   // On PPC ELF V2 abi, the first entry in the .got is the .TOC.
   unsigned GotHeaderEntriesNum = 0;
 
   bool NeedsThunks = false;
 
   // A 4-byte field corresponding to one or more trap instructions, used to pad
   // executable OutputSections.
-  uint32_t TrapInstr = 0;
+  std::array<uint8_t, 4> TrapInstr = {0, 0, 0, 0};
 
   // If a target needs to rewrite calls to __morestack to instead call
   // __morestack_non_split when a split-stack enabled caller calls a
   // non-split-stack callee this will return true. Otherwise returns false.
   bool NeedsMoreStackNonSplit = true;
 
   virtual RelExpr adjustRelaxExpr(RelType Type, const uint8_t *Data,
                                   RelExpr Expr) const;