[PATCH] D17269: [ELF] - Implemented linkerscript sections padding.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/InputSection.cpp:250-261
@@ +249,14 @@
+
+  // If there is some padding behind and value for padding is
+  // not null, this one fills the space repeating 4 bytes of
+  // padding value.
+  if (PadBehind && OutSec->PaddingValue) {
+    llvm::support::ubig32_t Bytes(OutSec->PaddingValue);
+    int I = OutSecOff - PadBehind;
+    while (PadBehind) {
+      Buf[OutSecOff - PadBehind] =
+          ((uint8_t *)&Bytes)[(OutSecOff - PadBehind - I) % 4];
+      --PadBehind;
+    }
+  }
+
----------------
grimar wrote:
> ruiu wrote:
> > This is not a right place to add this code.  You want to add code to OutputSection<ELFT>::writeTo instead. I think the following code should suffice as an initial implementation.
> > 
> >   +static void fill(uint8_t *Buf, size_t Size, ArrayRef<uint8_t> A) {
> >   +  size_t I = 0;
> >   +  for (; I < Size; I += A.size())
> >   +    memcpy(Buf + I, A.data(), A.size());
> >   +  memcpy(Buf + I, A.data(), Size - I);
> >   +}
> >   +
> >    template <class ELFT> void OutputSection<ELFT>::writeTo(uint8_t *Buf) {
> >   +  ArrayRef<uint8_t> Filler = Script->getFiller(Name);
> >   +  if (!Filler.empty())
> >   +    fill(Buf, getSize(), Filler);
> >      for (InputSection<ELFT> *C : Sections)
> >        C->writeTo(Buf);
> >    }
> Your code mixes the filling byters order it seems. I mean if we have two sections, 1 byte each with alignment 4, then if we have =0x11223344 filler:
> 
> My code will produce: [SEC1] 0x11 0x22 0x33 [SEC2].
> Your one will have [SEC1] 0x22 0x33 0x44 [SEC2].
> 
> I am not sure how much that is important as I saw fillers are used for NOPs which always were 0x90909090. If we are assuming that that is not important at all, then instead of fill() call we can use 
> 
> ```
> memset(Buf, Filler[0], getSize());
> ```
> 
> But if we want to support the order (and use all bytes), then I will adopt the algorithm. 
> Anyways I dont think we should use the fill() as it is now since it is somewhere in the middle.
> What do you think ?
I understand your point. As you might have implied, fill() should just work, so I'd like to go with that. That is way simpler than the current approach and probably faster (memcpy is usually pretty fast, which I experimented with Windows LLD.) I don't think setting the first byte is a good idea. NOP is one byte instruction on x86, but on RISC processors it is not. 


http://reviews.llvm.org/D17269