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

[Xiang Zhang via Phabricator via llvm-commits]

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

In D58102#1436187 <https://reviews.llvm.org/D58102#1436187>, @peter.smith wrote:

> I've checked that it does work, although at the moment it does leave a redundant .splt section in the binary. I think that this can be removed by accounting for this in the SPltSection::empty(), .........


It shouldn't have **redundant** .splt section, there should not have any .splt section if IBT is disable or non-dynamic link. Could you show me your test please?



================
Comment at: ELF/SyntheticSections.h:700
+  size_t getSize() const override;
+  bool empty() const override { return Entries.empty(); }
+
----------------
peter.smith wrote:
> If I've understood correctly if the .SPLT is disabled then this should return true, otherwise if you have any ifuncs then a redundant .splt section is produced. 
Till now this patch doesn't carefully handle ifunc, in ELF/Relocations.cpp:1072, I disable the splt if checked ifunc, if splt is disabled, there will be no splt section created.

```
  if (Config->SPltSectionEnable) {
    In.Splt = make<SPltSection>();
    Add(In.Splt);
  }
```


================
Comment at: test/ELF/x86-64-feature-1-and.s:57
+# NGPSPLT: SHSTK
+# DMPSPLT: splt
+
----------------
peter.smith wrote:
> The test checks that the splt is created, but it doesn't check that the contents are correct. There should be at least one of these tests that disassembles the .plt and checks that it is loading the correct .got entry.
> 
> There also should be a test to show that when an ifunc plt is added then we revert to using the .plt, and that any existing .splt entries are not called.
> 
> For example:
> 
> ```
> .type foo STT_GNU_IFUNC
> .globl ifunc
> ifun:
>  ret
> 
>  .globl func1
>  .type func1, at function
> func1:
>         callq func2
>         callq ifunc    // func2 will have been added to .splt, we don't want func3 to be.
>         callq func3
> ``` 
I check the contents of plts in my local, I'll add them to the patch.

For, I ifunc, I planned to handle it in my next patch, so I didn't add tests here. 


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D58102