[PATCH] D30886: [ELF] Pad x86 executable sections with 0xcc int3 instructions

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Generally looking good.



================
Comment at: ELF/LinkerScript.cpp:943
         return Cmd->Filler;
-  return 0;
+  return Optional<uint32_t>();
 }
----------------
Return `None`.


================
Comment at: ELF/OutputSections.cpp:239
 
+uint32_t OutputSection::getFill() {
+  // Determine what to fill gaps between InputSections with, as specified by the
----------------
This function doesn't use any member variable of OutputSection, so it should be a non-member function.


================
Comment at: ELF/OutputSections.cpp:244-248
+  uint32_t FillValue = 0;
+  if (Optional<uint32_t> Filler = Script->getFiller(this->Name))
+    FillValue = *Filler;
+  else if (Flags & SHF_EXECINSTR)
+    FillValue = Target->TrapInstr;
----------------
You can return early.

  if (Optional<uint32_t> Filler = Script->getFiller(this->Name))
    return *Filler;
  if (Flags & SHF_EXECINSTR)
    return Target->TrapInstr;
  return 0;


================
Comment at: ELF/OutputSections.cpp:257-264
+  if (!Sections.empty()) {
+    // Write any leading padding, in case the first input section does not
+    // appear at offset zero.
+    fill(Buf, Sections.front()->OutSecOff, FillValue);
+  } else {
+    // If the section has no input sections, fill it completely with the fill.
+    fill(Buf, this->Size, FillValue);
----------------
Not sure if we need to care about these edge cases. I wouldn't probably do anything for them to keep code simple.


================
Comment at: ELF/SyntheticSections.cpp:2183-2186
+  Optional<uint32_t> Fill = Script->getFiller(this->Name);
+  uint64_t Filler = Fill.getValueOr(0);
   Filler = (Filler << 32) | Filler;
   memcpy(Buf, &Filler, getSize());
----------------
If this is zero, this is no-op, right? If so, enclosing it with `if` is probably more natural.

  if (Optional<uint32_t> Filler = Script->getFiller(this->Name)) {
    ....
  }


https://reviews.llvm.org/D30886