[PATCH] D58102: Support X86 Control-flow Enforcement Technology (CET) in LLD

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm marked 3 inline comments as done.
xiangzhangllvm added a comment.

Hi ruiu, very thank you for considering accepting the 2-PLT ABI design in X86 arch, Yes, we really have no real measurable evidence for the benefit of this design, and I also think there must be other problems for the CET here, but it is just the beginning. We will work on improving and refining it.
Your questions and suggestions during the discussion were all very much to the point.
I'll first to implement your first 2 review suggestions above.
Thank you again!



================
Comment at: ELF/Driver.cpp:1444
 
+enum X86F1ANDForm {
+  X86F1ANDSingle,  // The section just contains X86F1AND info.
----------------
ruiu wrote:
> You added large amount of code here. What does this do in short? I'm not interested in a file format at the moment, but I'd like to know what this code is supposed to do at high level.
The main purpose of here code is collecting the X86 features in the .note.gnu.section. Because there are different formats of the .note.gnu.section, finding and collecting features need deal with some details here.  It really seems too much code, I'll try my best to simple here code.


================
Comment at: ELF/InputSection.cpp:710-722
+    // When the CET Branch protection is enabled, the second PLT
+    // should be call to.
+    if (Config->SPltSectionEnable)
+      return Sym.getSPltVA() + A;
+    else
+      return Sym.getPltVA() + A;
   case R_PLT_PC:
----------------
ruiu wrote:
> From the linker's perspective, `.splt` *is* PLT, which inter-module calls jump to. If `.splt` is enabled, `.plt` is no longer a PLT in the regular sense, but it just contains code to lazy-resolve symbols. So, I think you should handle `.splt` as the PLT despite its name throughout this patch if `.splt` is enabled. For example, `In.Plt` should be the .splt and `Sym.getPltVA()` should return its address in .splt.
> 
> (I feel that the spec made a mistake when naming .splt -- it should've been .plt and the current "first" PLT should be .splt, but it's probably too late to complain.)
Your suggestions make sense! I'll check and refine it, Thank you very much!


================
Comment at: ELF/Writer.cpp:1842
   finalizeSynthetic(In.Plt);
   finalizeSynthetic(In.Iplt);
+  finalizeSynthetic(In.Splt);
----------------
ruiu wrote:
> How is GNU ifunc supposed to work when CET is enabled?
The GNU IFUNC is also specified in https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf.
It very similar to lazy binding.
I think there will be not much problem for IFUNC if I use your second suggestion-(handle .splt as the PLT despite its name throughout this patch).
We'll resolve these un-compatibility between CET and IFUNC, retpolineplt in the following patches.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58102/new/

https://reviews.llvm.org/D58102