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

[George Rimar via llvm-commits]

grimar 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;
+    }
+  }
+
----------------
ruiu wrote:
> 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. 
Well, and the last means that version of fill() from above will not work as expected on RISC. It anyways will not work as expected for cases where one byte is not enough (because of shifted bytes). Moreover, what it worse, some times it _can_ work correctly, so that is random bug.

I don't argue about it is faster and simplier than current approach. Thats true. I just want to say that fill() should ideally be fixed to be correct and I am ready to do that. Since you insisted to use it as is, I did that in current patch iteration (just fixed the buffer overrun). 

Current implementation should at least have assert/error that checks that all 4 bytes are equal I think.


http://reviews.llvm.org/D17269