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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I asked about the rationale behind the 2-PLT scheme to H. J. Lu who proposed the design. I was not convinced one of the reasons he presented, which is that the 2-PLT scheme provides compatibility with old systems, because clearly the 2-PLT scheme breaks compatibility by changing the semantics of the existing .plt section and adding the second PLT. The other reason was more convincing, which is, it was designed with performance in mind, and it was an effort to reduce the size of the hot code path. With the 2-PLT scheme, the second PLT is in theory hot and the first PLT is relatively cold, as the first PLT is used only when a symbol is resolved lazily. That being said, there was not evidence or benchmark results supporting the claim, so I cannot really buy that argument.

So I'm not personally happy about that ABI. However, the 2-PLT scheme is there since Intel MPX support was added to the ABI, and it seems too late to propose a change. Therefore, as you and H. J. Lu suggested, we should proceed with the 2-PLT scheme. I'd like to leave a honest comment about the claimed design rationale and our thought for future reference, though (you don't need to do that in this patch.)



================
Comment at: ELF/Driver.cpp:1444
 
+enum X86F1ANDForm {
+  X86F1ANDSingle,  // The section just contains X86F1AND info.
----------------
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.


================
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:
----------------
>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.)


================
Comment at: ELF/Writer.cpp:1842
   finalizeSynthetic(In.Plt);
   finalizeSynthetic(In.Iplt);
+  finalizeSynthetic(In.Splt);
----------------
How is GNU ifunc supposed to work when CET is enabled?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D58102