Page MenuHomePhabricator

Support Intel Control-flow Enforcement Technology
ClosedPublic

Authored by MaskRay on Mar 25 2019, 10:19 AM.

Details

Summary

This patch is based on https://reviews.llvm.org/D58102.

This patch adds Intel CET (Control-flow Enforcement Technology) support to
lld. The implementation follows the draft version of psABI which you can
download from https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI.

I added a comment about what Intel CET is, so I won't repeat it here, but in
summary, CET introduces a new restriction on indirect jump instructions so
that you can limit the places to which you can jump to using indirect jumps.

In order to use the feature, you need to compile source files with
-fcf-protection=full. CET is enabled only when all input files are compiled with
the flag. If you want to make sure that the feature is enabled, pass
-z force-ibt -z shstk to lld.

CET-enabled executable files have two PLT sections, ".plt" and ".plt.sec".
For the details as to why we have two sections, please read this patch's
comment.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Ping. I'm planning to upstream support for the AArch64 BTI and PAC features in LLD, llvm-objdump and llvm-readelf. I've written the LLD patches on top of this patch as the .note.gnu.property section is used in a similar way (GNU_PROPERTY_AARCH64_FEATURE_1_AND) so only a few small alterations are needed to that part of the code. I'm wondering if it is best to:

  • Submit the patches rebased on top of this patch.
    • The advantage here is that you could see the differences in supporting X86 and AArch64 more clearly.
  • Strip out the .note.gnu.property code from this patch and resubmit it as part of the AArch64 BTI and PAC.
    • The advantage here is that the AArch64 feature doesn't have to wait for CET to land.

Any suggestions or preferences? I have the code written and will start upstreaming tomorrow. There are some LLVM for llvm-readobj and llvm-objdump that the LLD patches will depend on so I'll start with those.

The ELF ABI has details on the .note.gnu.property support. https://developer.arm.com/docs/ihi0056/latest/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q1-documentation.

I was also wondering if there has been any progress with this. Was there any more discussion on the ABI mailing list?

I'm sorry, I reflect the problem to some leaders and experts, but the PLT ABI is still undecided. The most concern come from the unknow risk of difference with GNU.

I'm sorry, I reflect the problem to some leaders and experts, but the PLT ABI is still undecided. The most concern come from the unknow risk of difference with GNU.

@xiangzhangllvm If it is still undecided, then there is still opportunity to fix :) IMHO tooling is still immature and currently very few tools understand .plt.sec.

I'd really like to see more discussions about this, e.g. https://groups.google.com/forum/#!topic/x86-64-abi/Z6GqQDy_NaI
If PLT entries should be aligned by 16 to maximize performance, shall we switch to 32-byte PLT entry? Xiang, can you raise a topic on https://sourceware.org/ml/gnu-gabi/2019-q2 or https://sourceware.org/ml/binutils/2019-05/

I'm sorry, I reflect the problem to some leaders and experts, but the PLT ABI is still undecided. The most concern come from the unknow risk of difference with GNU.

@xiangzhangllvm If it is still undecided, then there is still opportunity to fix :) IMHO tooling is still immature and currently very few tools understand .plt.sec.

I'd really like to see more discussions about this, e.g. https://groups.google.com/forum/#!topic/x86-64-abi/Z6GqQDy_NaI
If PLT entries should be aligned by 16 to maximize performance, shall we switch to 32-byte PLT entry? Xiang, can you raise a topic on https://sourceware.org/ml/gnu-gabi/2019-q2 or https://sourceware.org/ml/binutils/2019-05/

hi, fangrui, 32-byte PLT entry is also aligned by 16, I have test the performance before, they are same.

MaskRay added a comment.EditedMay 28 2019, 8:33 PM

I'm sorry, I reflect the problem to some leaders and experts, but the PLT ABI is still undecided. The most concern come from the unknow risk of difference with GNU.

@xiangzhangllvm If it is still undecided, then there is still opportunity to fix :) IMHO tooling is still immature and currently very few tools understand .plt.sec.

I'd really like to see more discussions about this, e.g. https://groups.google.com/forum/#!topic/x86-64-abi/Z6GqQDy_NaI
If PLT entries should be aligned by 16 to maximize performance, shall we switch to 32-byte PLT entry? Xiang, can you raise a topic on https://sourceware.org/ml/gnu-gabi/2019-q2 or https://sourceware.org/ml/binutils/2019-05/

hi, fangrui, 32-byte PLT entry is also aligned by 16, I have test the performance before, they are same.

Great. If the unified 32-byte PLT entry scheme doesn't regress, let's delete .plt.sec and fix the ABI. A simplified scheme will ease the adoption of CET. I guess you are in a good position to do this. You may send an email to the gnu-gabi and binutils mailing lists.

The current problem is the 2nd PLT ABI was in GCC side for more than 2 years. I think we need to work with GCC community to have a consistent ABI so we won't break each other. Let's have more discussion in x86_64 ABI site.

The current problem is the 2nd PLT ABI was in GCC side for more than 2 years. I think we need to work with GCC community to have a consistent ABI so we won't break each other. Let's have more discussion in x86_64 ABI site.

@annita.zhang Thanks for weighing in! AFAIK, .plt.sec is a section synthesized by the linker. GCC doesn't know or have to know how PLT entries are represented.

for more than 2 years

This is less of a concern to me. Even GNU objdump compiled from binutils-gdb trunk (the same repository as ld.bfd) doesn't know how to symbolize such .plt.sec or .plt entries, so I believe the tooling is still immature and the adoption is low.

Annita, you and Xiang may have better ideas which parties should be involved in the discussion. Could one of you start a thread on x86-64-abi, binutils, and gnu-gabi? And/or gcc (if you think GCC folks should also be involved). You may CC Rui, Peter, and me.

ruiu added a comment.May 29 2019, 5:45 AM

I was planning to start a discussion at the gabi mailing list. I noticed that I didn't join the mailing list, and it's moderated, so I waited my join request would be approved, and then when it was approved, I didn't start a discussion immediately. Sorry for my negligence. Let me start a discussion.

In lld we already support two different PLT formats for mitigating Spectre. It might make sense to add them to the standard if enough number of people are already using them. Last I tried it with gdb, gdb crashes because of the nonstandard PLT format.

MaskRay added inline comments.May 30 2019, 3:24 AM
lld/ELF/Config.h
89

Split off the X86Features (or ANDFeatures) change and commit, to make D62609 easier to review?

ruiu added a comment.Jun 3 2019, 6:37 AM

I sent a proposal to the x86-64-abi mailing list. You can read the thread here: https://groups.google.com/forum/?hl=en#!topic/x86-64-abi/sQcX3__r4c0

ruiu updated this revision to Diff 203536.Jun 7 2019, 4:58 AM
  • rebase so that the patch does not diverge too much from the trunk
ruiu updated this revision to Diff 203771.Jun 9 2019, 10:20 PM
  • fix tests

Hi,

I added a comment to x86-64 ABI google group link. I also posted it below.

This CET ABI issue was raised during glibc discussion in GCC Cauldron. The conclusion was CET ABI wouldn’t be changed. It’s not because it’s a better option, but because it’s been finalized for 2 years (in 2017). And the implementation has been included in Linux distributions already. There's no obvious reason to change it w/o a pretty better solution.

On the other hand, Intel wants to support it in LLVM lld anyway. But we don’t want to force lld developers to do what they don’t want. So now the ball is at lld side. If lld would like to follow the CET ABI, we’re willing to submit the corresponding lld patches. If not, we will submit the lld patches supporting 32-byte PLTs. The disadvantage of the latter is ABI is incompatible between GCC and LLVM for CET. It may cause potential issues in some tools which has dependence on PLT size and layout. But we don’t have known samples yet. From our estimation, the risk may be low.

I’m looking forward to hearing your opinions and moving it forward.

Thanks,
Annita

MaskRay accepted this revision.EditedOct 16 2019, 2:38 AM

Hi,

I added a comment to x86-64 ABI google group link. I also posted it below.

This CET ABI issue was raised during glibc discussion in GCC Cauldron. The conclusion was CET ABI wouldn’t be changed. It’s not because it’s a better option, but because it’s been finalized for 2 years (in 2017). And the implementation has been included in Linux distributions already. There's no obvious reason to change it w/o a pretty better solution.

On the other hand, Intel wants to support it in LLVM lld anyway. But we don’t want to force lld developers to do what they don’t want. So now the ball is at lld side. If lld would like to follow the CET ABI, we’re willing to submit the corresponding lld patches. If not, we will submit the lld patches supporting 32-byte PLTs. The disadvantage of the latter is ABI is incompatible between GCC and LLVM for CET. It may cause potential issues in some tools which has dependence on PLT size and layout. But we don’t have known samples yet. From our estimation, the risk may be low.

I’m looking forward to hearing your opinions and moving it forward.

Thanks,
Annita

I actually made a similar proposal https://groups.google.com/forum/?hl=en#!topic/x86-64-abi/Z6GqQDy_NaI in April but it did not spur discussions.
Ali Bahrami and Michael Matz were active in generic-abi discussions. I am really glad they commented on https://groups.google.com/forum/?hl=en#!topic/x86-64-abi/sQcX3__r4c0 My summary is that many people think .plt.sec is overengineered. Carlos's stance is fairly obvious from the discussion. It is just too late to fix anything. (Gossip is that it is probably only Carlos himself cares about the auditing feature in glibc... As people have pointed out, very few libc implementations (just glibc and Solaris libc) support auditing so this does not surprise me at all -;))

In the Cauldron, GCC/LLVM Collaboration BoF was a topic. The PLT scheme argument reflects a bigger problem that some collaboration does not work well. I will give an example about the latest binutils 2.33.1 release. I caught the objcopy --set-section-alignment problem in the last minute (https://sourceware.org/ml/binutils/2019-10/msg00008.html). I would regret if the problematic objcopy patch slipped through and GNU developers were reluctant to change it - which would mean llvm-objcopy would not have an ideal solution. Recently in another case, I caught a problem related to .ctf but I guess GNU maintainers will not care my opinion. I'll stop as it may be too off-topic now.

For my own notes, I should follow binutils, libc-alpha, and x86-64-abi discussions more closely.

I accepted this patch. I am still sad about the .plt.sec scheme but am not opposed to this patch. It is up to @ruiu to rebase the patch and commit it.

This comment was removed by annita.zhang.

In the Cauldron, GCC/LLVM Collaboration BoF was a topic. The PLT scheme argument reflects a bigger problem that some collaboration does not work well. I will give an example about the latest binutils 2.33.1 release. I caught the objcopy --set-section-alignment problem in the last minute (https://sourceware.org/ml/binutils/2019-10/msg00008.html). I would regret if the problematic objcopy patch slipped through and GNU developers were reluctant to change it - which would mean llvm-objcopy would not have an ideal solution. Recently in another case, I caught a problem related to .ctf but I guess GNU maintainers will not care my opinion. I'll stop as it may be too off-topic now.

For my own notes, I should follow binutils, libc-alpha, and x86-64-abi discussions more closely.

I accepted this patch. I am still sad about the .plt.sec scheme but am not opposed to this patch. It is up to @ruiu to rebase the patch and commit it.

@MaskRay, I totally agree with you that LLVM and GCC community should work more closely on those ABI standards. For example, we are facing some LLVM ABI noncompliant issues especially in legacy ia32. And they're hard to fix. We need to involve both LLVM and GCC into discussion for future ABI changes. So both can be ABI compliant once it's finalized.

@ruiu, what's your decision then?

ruiu added a comment.Nov 8 2019, 2:43 AM

This patch is outdated and needs rebasing. I'll rebase, upload it again and then submit.

Sounds good. Thank you!

This patch is outdated and needs rebasing. I'll rebase, upload it again and then submit.

Hi ruiu, just remind, if you have no time, I can rebase it.

This patch is outdated and needs rebasing. I'll rebase, upload it again and then submit.

Hi ruiu, just remind, if you have no time, I can rebase it.

I can do that, too, and cleaning the test.

lld/test/ELF/x86-64-cet.s
13

These instructions should be aligned.

llvm-objcopy<9 may have alignment problems.

This patch is outdated and needs rebasing. I'll rebase, upload it again and then submit.

Hi ruiu, just remind, if you have no time, I can rebase it.

I can do that, too, and cleaning the test.

Very good! thank you! In fact, I am a little unfamiliar about LLD code, for I have not do the lld work for a long time.

MaskRay updated this revision to Diff 233253.Dec 10 2019, 8:13 PM

Rebase and improve comments.

ruiu added a comment.Dec 10 2019, 8:39 PM

Hmm, I'm sorry but I'm confused. IIRC I had a discussion in the LLVM dev meeting that we were going to submit a change with a single PLT scheme rather than IPLT, so when I said that I'm going to submit a patch, I meant that I'm going to submit a patch for the 1PLT scheme rather than the 2PLT scheme. But this is for the 2PLT scheme. This is not something I want.

MaskRay added a comment.EditedDec 10 2019, 8:53 PM

@xiangzhangllvm @annita.zhang The patch has been rebased, but it doesn't seem to work.

I made a local patch to make --require-cet behave more like --force-bti:

--- i/lld/ELF/Driver.cpp
+++ w/lld/ELF/Driver.cpp
@@ -1705,2 +1705,4 @@ template <class ELFT> static uint32_t getAndFeatures() {
-    } else if (!features && config->requireCET)
-      error(toString(f) + ": --require-cet: file is not compatible with CET");
+    } else if (config->requireCET && !(features & GNU_PROPERTY_X86_FEATURE_1_IBT)) {
+      warn(toString(f) + ": --require-cet: file is not compatible with CET");
+      features |= GNU_PROPERTY_X86_FEATURE_1_IBT;
+    }

I tried a trivial program with 2 PLT calls.

gcc -fcf-protection=full -c a.c
gcc -fcf-protection=full a.c -o a '-###'   # Retrieve linker command line, replace ld with

My GCC crt files are not CET compatible but I think that is probably irrelevant.

% ld.lld --eh-frame-hdr -m elf_x86_64 "--hash-style=gnu" -dynamic-linker /lib64/ld-linux-x86-64.so.2 -pie -o a /usr/lib/gcc/x86_64-linux-gnu/8/../../
../x86_64-linux-gnu/Scrt1.o /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/crti.o /usr/lib/gcc/x86_64-linux-gnu/8/crtbeginS.o -L/usr/lib/gc
c/x86_64-linux-gnu/8 -L/usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu -L/usr/lib/gcc/x86_64-linux-gnu/8/../../../../lib -L/lib/x86_64-linux
-gnu -L/lib/../lib -L/usr/lib/x86_64-linux-gnu -L/usr/lib/../lib -L/usr/lib/gcc/x86_64-linux-gnu/8/../../.. a.o -lgcc --push-state --as-needed -lgcc_s
 --pop-state -lc -lgcc --push-state --as-needed -lgcc_s --pop-state /usr/lib/gcc/x86_64-linux-gnu/8/crtendS.o /usr/lib/gcc/x86_64-linux-gnu/8/../../..
/x86_64-linux-gnu/crtn.o --require-cet -o a                                                                                                          
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/Scrt1.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/crti.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/crtbeginS.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/x86_64-linux-gnu/libc_nonshared.a(elf-init.oS): --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/crtendS.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/crtn.o: --require-cet: file is not compatible with CET

OK, it segfaults. So there may be some issues in the PLT.

./a => segmentation fault

Peter Smith implemented --force-bti for AArch64 in D62609.
Its semantic is:

--force-bti : Act as if all relocatable inputs had GNU_PROPERTY_AARCH64_FEATURE_1_BTI and warn for every relocatable object that does not.

Do you think it makes more sense to change --require-cet to --force-cet or --force-ibt?

There is also some glibc code

@peter.smith I find that in binutils-gdb, the AArch64 option is named -z force-bti, not --force-bti (commit [BFD, LD, AArch64, 2/3] Add --force-bti to enable BTI and to select BTI enabled PLTs). Do you intend to make them consistent?

Hmm, I'm sorry but I'm confused. IIRC I had a discussion in the LLVM dev meeting that we were going to submit a change with a single PLT scheme rather than IPLT, so when I said that I'm going to submit a patch, I meant that I'm going to submit a patch for the 1PLT scheme rather than the 2PLT scheme. But this is for the 2PLT scheme. This is not something I want.

What was the decision that was made at the developer meeting? Will lld support the 2PLT scheme defined in the psABI?

MaskRay added a comment.EditedDec 10 2019, 9:16 PM

Hmm, I'm sorry but I'm confused. IIRC I had a discussion in the LLVM dev meeting that we were going to submit a change with a single PLT scheme rather than IPLT, so when I said that I'm going to submit a patch, I meant that I'm going to submit a patch for the 1PLT scheme rather than the 2PLT scheme. But this is for the 2PLT scheme. This is not something I want.

What was the decision that was made at the developer meeting? Will lld support the 2PLT scheme defined in the psABI?

(Personally, I am still unhappy with the .plt.sec scheme. I think I prefer a 1PLT scheme like AArch64's Branch Target Indicators (BTI) and Pointer Authentication Code (PAC).) But I thought @ruiu accepted the fait accompli because there had been very strong objection to diverge from what GNU ld does. https://groups.google.com/forum/#!topic/x86-64-abi/sQcX3__r4c0

objdump -d (built from binutils-gdb HEAD) still does not work with lld produced binaries.

% objdump -d a.bfd  # -z ibt
Disassembly of section .plt:

0000000000001020 <.plt>:
    1020:       ff 35 e2 2f 00 00       pushq  0x2fe2(%rip)        # 4008 <_GLOBAL_OFFSET_TABLE_+0x8>
    1026:       f2 ff 25 e3 2f 00 00    bnd jmpq *0x2fe3(%rip)        # 4010 <_GLOBAL_OFFSET_TABLE_+0x10>
    102d:       0f 1f 00                nopl   (%rax)
    1030:       f3 0f 1e fa             endbr64 
    1034:       68 00 00 00 00          pushq  $0x0
    1039:       f2 e9 e1 ff ff ff       bnd jmpq 1020 <.plt>
    103f:       90                      nop
    1040:       f3 0f 1e fa             endbr64 
    1044:       68 01 00 00 00          pushq  $0x1
    1049:       f2 e9 d1 ff ff ff       bnd jmpq 1020 <.plt>
    104f:       90                      nop
Disassembly of section .plt.got:

0000000000001050 <__cxa_finalize@plt>:
    1050:       f3 0f 1e fa             endbr64 
    1054:       f2 ff 25 9d 2f 00 00    bnd jmpq *0x2f9d(%rip)        # 3ff8 <__cxa_finalize@GLIBC_2.2.5>
    105b:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)

Disassembly of section .plt.sec:

0000000000001060 <puts@plt>:
    1060:       f3 0f 1e fa             endbr64 
    1064:       f2 ff 25 ad 2f 00 00    bnd jmpq *0x2fad(%rip)        # 4018 <puts@GLIBC_2.2.5>
    106b:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)

0000000000001070 <exit@plt>:
    1070:       f3 0f 1e fa             endbr64 
    1074:       f2 ff 25 a5 2f 00 00    bnd jmpq *0x2fa5(%rip)        # 4020 <exit@GLIBC_2.2.5>
    107b:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
% objdump -d a.lld  # --require-cet (actually my patched version, which behaves like AArch64's --force-bti
...
Disassembly of section .plt:

00000000000018c0 <.plt>:
    18c0:       ff 35 0a 22 00 00       pushq  0x220a(%rip)        # 3ad0 <__init_array_end+0x1190>
    18c6:       ff 25 0c 22 00 00       jmpq   *0x220c(%rip)        # 3ad8 <__init_array_end+0x1198>
    18cc:       0f 1f 40 00             nopl   0x0(%rax)
    18d0:       f3 0f 1e fa             endbr64 
    18d4:       68 00 00 00 00          pushq  $0x0
    18d9:       e9 e2 ff ff ff          jmpq   18c0 <_fini+0x14>
    18de:       66 90                   xchg   %ax,%ax
    18e0:       f3 0f 1e fa             endbr64 
    18e4:       68 01 00 00 00          pushq  $0x1
    18e9:       e9 d2 ff ff ff          jmpq   18c0 <_fini+0x14>
    18ee:       66 90                   xchg   %ax,%ax
    18f0:       f3 0f 1e fa             endbr64 
    18f4:       68 02 00 00 00          pushq  $0x2
    18f9:       e9 c2 ff ff ff          jmpq   18c0 <_fini+0x14>
    18fe:       66 90                   xchg   %ax,%ax

Disassembly of section .plt.sec:

0000000000001900 <.plt.sec>:
    1900:       f3 0f 1e fa             endbr64 
    1904:       ff 25 16 22 00 00       jmpq   *0x2216(%rip)        # 3b20 <__cxa_finalize@GLIBC_2.2.5>
    190a:       66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)
    1910:       f3 0f 1e fa             endbr64 
    1914:       ff 25 0e 22 00 00       jmpq   *0x220e(%rip)        # 3b28 <puts@GLIBC_2.2.5>
    191a:       66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)
    1920:       f3 0f 1e fa             endbr64 
    1924:       ff 25 06 22 00 00       jmpq   *0x2206(%rip)        # 3b30 <exit@GLIBC_2.2.5>
    192a:       66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)

On AArch64, the linker produces two dynamic tags DT_AARCH64_BTI_PLT and DT_AARCH64_PAC_PLT which will be read by glibc ld.so. I haven't checked how x86_64 glibc handles this.

I will probably build a top-of-tree glibc and a top-of-tree gcc tomorrow and investigate more about these stuff.

Hmm, I'm sorry but I'm confused. IIRC I had a discussion in the LLVM dev meeting that we were going to submit a change with a single PLT scheme rather than IPLT, so when I said that I'm going to submit a patch, I meant that I'm going to submit a patch for the 1PLT scheme rather than the 2PLT scheme. But this is for the 2PLT scheme. This is not something I want.

What was the decision that was made at the developer meeting? Will lld support the 2PLT scheme defined in the psABI?

I was confused too. @ruiu, maskray, what's the decision for lld support to CET?

xiangzhangllvm added inline comments.Dec 10 2019, 9:46 PM
lld/ELF/Driver.cpp
1704–1709

I don't know why "--forece-bti" use "warn" not "error", in my eyes, people use --require-cet to generate the output file, mostly means, they want to run the output file on a CET-checked machine. It will run fail when linked Non-CET object files to the output file. So here we used "error" instead of "warn".

MaskRay added inline comments.Dec 10 2019, 9:59 PM
lld/ELF/Driver.cpp
1704–1709

GNU ld supports -z cet-report=[none|warning|error] (added in 2019-04) https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=233a00833b984319d5e94db3f5d5d9a735edc984

Examples:

% ld.bfd ... -z cet-report=error              # exit code: 1
./ld-new: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/Scrt1.o: error: missing SHSTK property
./ld-new: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/crti.o: error: missing SHSTK property
% ld.bfd ... -z cet-report=warning        # exit code: 0
./ld-new: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/Scrt1.o: warning: missing SHSTK property
./ld-new: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/crti.o: warning: missing SHSTK property
> gcc -fcf-protection=full -c a.c
> gcc -fcf-protection=full a.c -o a '-###'   # Retrieve linker command line, replace ld with

My GCC crt files are not CET compatible but I think that is probably irrelevant.

% ld.lld --eh-frame-hdr -m elf_x86_64 "--hash-style=gnu" -dynamic-linker /lib64/ld-linux-x86-64.so.2 -pie -o a /usr/lib/gcc/x86_64-linux-gnu/8/../../
../x86_64-linux-gnu/Scrt1.o /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/crti.o /usr/lib/gcc/x86_64-linux-gnu/8/crtbeginS.o -L/usr/lib/gc
c/x86_64-linux-gnu/8 -L/usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu -L/usr/lib/gcc/x86_64-linux-gnu/8/../../../../lib -L/lib/x86_64-linux
-gnu -L/lib/../lib -L/usr/lib/x86_64-linux-gnu -L/usr/lib/../lib -L/usr/lib/gcc/x86_64-linux-gnu/8/../../.. a.o -lgcc --push-state --as-needed -lgcc_s
 --pop-state -lc -lgcc --push-state --as-needed -lgcc_s --pop-state /usr/lib/gcc/x86_64-linux-gnu/8/crtendS.o /usr/lib/gcc/x86_64-linux-gnu/8/../../..
/x86_64-linux-gnu/crtn.o --require-cet -o a                                                                                                          
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/Scrt1.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/crti.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/crtbeginS.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/x86_64-linux-gnu/libc_nonshared.a(elf-init.oS): --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/crtendS.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib/gcc/x86_64-linux-gnu/8/../../../x86_64-linux-gnu/crtn.o: --require-cet: file is not compatible with CET

OK, it segfaults. So there may be some issues in the PLT.

./a => segmentation fault

Can you share your a.c code?
I test the "hello world" test, it is ok. (At first I use as 2.25, it failed at assembling.)

[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$gcc -fcf-protection=full main.c -S -o main.s
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$cat main.c
#include<stdio.h>
int main() {
 printf("Hello llvm!\n");
  return 0;
}
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$gcc -fcf-protection=full main.c -S -o main.s
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$/rdrive/ref/binutils/2.29/rhel70/efi2/bfd/bin/as --64 main.s -o main.o
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$/rdrive/ref/binutils/2.29/rhel70/efi2/bfd/bin/as -version
GNU assembler (GNU Binutils) 2.29
Copyright (C) 2017 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or later.
This program has absolutely no warranty.
This assembler was configured for a target of `x86_64-linux-gnu'.
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$../build-cet/bin/ld.lld --require-cet -plugin /nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../libexec/gcc/x86_64-linux-gnu/8.3.0/liblto_plugin.so "-plugin-opt=/nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../libexec/gcc/x86_64-linux-gnu/8.3.0/lto-wrapper" "-plugin-opt=-fresolution=/tmp/ccQKvMnE.res" "-plugin-opt=-pass-through=-lgcc" "-plugin-opt=-pass-through=-lgcc_s" "-plugin-opt=-pass-through=-lc" "-plugin-opt=-pass-through=-lgcc" "-plugin-opt=-pass-through=-lgcc_s" --eh-frame-hdr -m elf_x86_64 -dynamic-linker /lib64/ld-linux-x86-64.so.2 -o a.out /lib/../lib64/crt1.o /lib/../lib64/crti.o /nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/crtbegin.o -L/nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0 -L/nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc -L/nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/../../../../lib64 -L/lib/../lib64 -L/usr/lib/../lib64 -L/nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/../../.. main.o -lgcc --as-needed -lgcc_s --no-as-needed -lc -lgcc --as-needed -lgcc_s --no-as-needed /nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/crtend.o /lib/../lib64/crtn.o
ld.lld: warning: /lib/../lib64/crt1.o: --require-cet: file is not compatible with CET
ld.lld: warning: /lib/../lib64/crti.o: --require-cet: file is not compatible with CET
ld.lld: warning: /nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/crtbegin.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib64/libc_nonshared.a(elf-init.oS): --require-cet: file is not compatible with CET
ld.lld: warning: /nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/crtend.o: --require-cet: file is not compatible with CET
ld.lld: warning: /lib/../lib64/crtn.o: --require-cet: file is not compatible with CET
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$./a.out
Hello llvm!
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$

Remove the "-plugin xxx", also work.

[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$../build-cet/bin/ld.lld --require-cet  --eh-frame-hdr -m elf_x86_64 -dynamic-linker /lib64/ld-linux-x86-64.so.2 -o a.out /lib/../lib64/crt1.o /lib/../lib64/crti.o /nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/crtbegin.o -L/nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0 -L/nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc -L/nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/../../../../lib64 -L/lib/../lib64 -L/usr/lib/../lib64 -L/nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/../../.. main.o -lgcc --as-needed -lgcc_s --no-as-needed -lc -lgcc --as-needed -lgcc_s --no-as-needed /nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/crtend.o /lib/../lib64/crtn.o                                                                           ld.lld: warning: /lib/../lib64/crt1.o: --require-cet: file is not compatible with CET
ld.lld: warning: /lib/../lib64/crti.o: --require-cet: file is not compatible with CET
ld.lld: warning: /nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/crtbegin.o: --require-cet: file is not compatible with CET
ld.lld: warning: /usr/lib64/libc_nonshared.a(elf-init.oS): --require-cet: file is not compatible with CET
ld.lld: warning: /nfs/sc/proj/icl/rdrive/ref/gcc/8.3.0/rhel70/efi2/bin/../lib/gcc/x86_64-linux-gnu/8.3.0/crtend.o: --require-cet: file is not compatible with CET
ld.lld: warning: /lib/../lib64/crtn.o: --require-cet: file is not compatible with CET
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$./a.out
Hello llvm!
[xiangzh1@scels75 /export/iusers/xiangzh1/LLVM/ORG/test]$
MaskRay updated this revision to Diff 233263.EditedDec 11 2019, 12:24 AM

Apply two patches

-  uint64_t Plt = In.Plt->getVA();
+  uint64_t Plt = In.IBTPlt ? In.IBTPlt->getVA() : In.Plt->getVA();
--- i/lld/ELF/Driver.cpp
+++ w/lld/ELF/Driver.cpp
@@ -1705,2 +1705,4 @@ template <class ELFT> static uint32_t getAndFeatures() {
-    } else if (!features && config->requireCET)
-      error(toString(f) + ": --require-cet: file is not compatible with CET");
+    } else if (config->requireCET && !(features & GNU_PROPERTY_X86_FEATURE_1_IBT)) {
+      warn(toString(f) + ": --require-cet: file is not compatible with CET");
+      features |= GNU_PROPERTY_X86_FEATURE_1_IBT;
+    }

@xiangzhangllvm The previous version I uploaded definitely did not work. I just applied a fix. Both i386 and x86_64 seem to work now, but there are still problems with the tests.

You need either arc patch D59780 or curl -L 'https://reviews.llvm.org/D59780?download=true' | patch -p1 to apply the latest diff in your local repository.

I have a separate patch to rename lld --force-bti to -z force-bti. I think --force-ibt or -z force-ibt may make more sense than --require-cet.

Hmm, I'm sorry but I'm confused. IIRC I had a discussion in the LLVM dev meeting that we were going to submit a change with a single PLT scheme rather than IPLT, so when I said that I'm going to submit a patch, I meant that I'm going to submit a patch for the 1PLT scheme rather than the 2PLT scheme. But this is for the 2PLT scheme. This is not something I want.

What was the decision that was made at the developer meeting? Will lld support the 2PLT scheme defined in the psABI?

I don't know what was discussed at the developer meeting, but I don't see how not following the established ABI is an option. It seems clear that GNU implementations will not change the ABI they've been using for 2+ years. I don't see the value in having lld create a competing ABI.

Apply two patches

-  uint64_t Plt = In.Plt->getVA();
+  uint64_t Plt = In.IBTPlt ? In.IBTPlt->getVA() : In.Plt->getVA();
--- i/lld/ELF/Driver.cpp
+++ w/lld/ELF/Driver.cpp
@@ -1705,2 +1705,4 @@ template <class ELFT> static uint32_t getAndFeatures() {
-    } else if (!features && config->requireCET)
-      error(toString(f) + ": --require-cet: file is not compatible with CET");
+    } else if (config->requireCET && !(features & GNU_PROPERTY_X86_FEATURE_1_IBT)) {
+      warn(toString(f) + ": --require-cet: file is not compatible with CET");
+      features |= GNU_PROPERTY_X86_FEATURE_1_IBT;
+    }

@xiangzhangllvm The previous version I uploaded definitely did not work. I just applied a fix. Both i386 and x86_64 seem to work now, but there are still problems with the tests.

You need either arc patch D59780 or curl -L 'https://reviews.llvm.org/D59780?download=true' | patch -p1 to apply the latest diff in your local repository.

I have a separate patch to rename lld --force-bti to -z force-bti. I think --force-ibt or -z force-ibt may make more sense than --require-cet.

Thank you very much!
I checked the "X86-64 fail tests", the reason is just that you changed "error" to "warn".
--force-ibt maybe use, (but to change --force-ibt to --force-cet, because 'cet' contains 'ibt' and 'shstk')
--require-cet is also needed, (strictly check the input files, make sure run ok in CET machines).
And I think it better to add --try-cet too, try to generate CET output if all inputs is CETed, this must be useful in bootstrap compiling program. (To let the whole program CETed step by step).

Any way, we can add the other "refined options" late, this time, just let '-force-cet' in.

Apply two patches

-  uint64_t Plt = In.Plt->getVA();
+  uint64_t Plt = In.IBTPlt ? In.IBTPlt->getVA() : In.Plt->getVA();
--- i/lld/ELF/Driver.cpp
+++ w/lld/ELF/Driver.cpp
@@ -1705,2 +1705,4 @@ template <class ELFT> static uint32_t getAndFeatures() {
-    } else if (!features && config->requireCET)
-      error(toString(f) + ": --require-cet: file is not compatible with CET");
+    } else if (config->requireCET && !(features & GNU_PROPERTY_X86_FEATURE_1_IBT)) {
+      warn(toString(f) + ": --require-cet: file is not compatible with CET");
+      features |= GNU_PROPERTY_X86_FEATURE_1_IBT;
+    }

@xiangzhangllvm The previous version I uploaded definitely did not work. I just applied a fix. Both i386 and x86_64 seem to work now, but there are still problems with the tests.

You need either arc patch D59780 or curl -L 'https://reviews.llvm.org/D59780?download=true' | patch -p1 to apply the latest diff in your local repository.

I have a separate patch to rename lld --force-bti to -z force-bti. I think --force-ibt or -z force-ibt may make more sense than --require-cet.

Thank you very much!
I checked the "X86-64 fail tests", the reason is just that you changed "error" to "warn".

`--force-ibt` maybe use, (but to change `--force-ibt` to `--force-cet`, because 'cet' contains 'ibt' and 'shstk')

--require-cet is also needed, (strictly check the input files, make sure run ok in CET machines).
And I think it better to add --try-cet too, try to generate CET output if all inputs is CETed, this must be useful in bootstrap compiling program. (To let the whole program CETed step by step).

Any way, we can add the other "refined options" late, this time, just let '-force-cet' in.

x86 IBT is similar to AArch64 BTI. Whether we should upgrade from -z force-ibt to -z force-cet depends on whether shstk requires support in the object files for correctness.. Here is my understanding.

  • GNU_PROPERTY_X86_FEATURE_1_IBT represents the capability of IBT in an object file. The output is IBT capable only if all object files have the bit. GNU_PROPERTY_AARCH64_FEATURE_1_BTI belongs to this category. So naming the relevant option -z force-ibt makes sense.
  • GNU_PROPERTY_X86_FEATURE_1_SHSTK represents that an object file desires SHSTK. It does not require support in the object files for correctness. GNU_PROPERTY_AARCH64_FEATURE_1_PAC_PLT belongs to this category.

I would name the two options -z force-ibt and -z shstk. I can imagine that shstk may have non-negligible overhead, and probably not everyone wants to enable it. So having separate options makes sense.

(GNU ld supports -z ibt and -z cet-report=[none|warning|error]. If shstk, as I think, does not require object files' support, I will consider it not so ideal. The name -z ibt does not make it clear that this forces ibt when some object files are not IBT capable. -z cet-report mixes intentions of two features.)

If @ruiu still accepts the patch in the first place, I may go with -z force-ibt and -z shstk.

MaskRay added a comment.EditedDec 11 2019, 7:45 PM

Hmm, I'm sorry but I'm confused. IIRC I had a discussion in the LLVM dev meeting that we were going to submit a change with a single PLT scheme rather than IPLT, so when I said that I'm going to submit a patch, I meant that I'm going to submit a patch for the 1PLT scheme rather than the 2PLT scheme. But this is for the 2PLT scheme. This is not something I want.

What was the decision that was made at the developer meeting? Will lld support the 2PLT scheme defined in the psABI?

I don't know what was discussed at the developer meeting, but I don't see how not following the established ABI is an option. It seems clear that GNU implementations will not change the ABI they've been using for 2+ years. I don't see the value in having lld create a competing ABI.

The PLT scheme in this patch is already different. See that GNU ld -z ibt produces a MPX bnd prefix but this patch doesn't. Also check out my objdump (built from HEAD) example above. PLT schemes have already diverged. I am still not very convinced doing things differently is breaking ABI. If pursing for 1PLT scheme breaks ABI, then this patch breaks ABI, too. In the long term, downstream tools should be taught not to rely on the exact forms of PLT entries. They should make assumption that the first instructions takes X bytes, the second takes Y, etc. They should use better heuristics and the psABI should mention this so that we won't get locked into the any specific PLT scheme.

For MPX prefix:
GCC have not supported the MPX from GCC 9. And intel will not support MPX code too. So we don’t consider MPX for CET in LLD.

For MPX prefix:
GCC have not supported the MPX from GCC 9. And intel will not support MPX code too. So we don’t consider MPX for CET in LLD.

I know that GCC has removed MPX and the Linux kernel is removing MPX (user-visible APIs and self-tests have been removed). I asked because I haven't seen a change on binutils-gdb side that will support a .plt.sec scheme without the BND prefix. So, I wonder what kind of changes are considered divergence from x86-64 psABI. After the removal of the BND prefix, the .plt entry will get the leeway of 2 bytes. If, say, in the future, a new security enhanced feature is proposed which requires a new instruction that will take more than 2 bytes, the 16-byte .plt entry no longer works, and toolchains will have to migrate a third PLT scheme, different from traditional PLT and the .plt.sec scheme.

As to the option name question, are you happy with -z force-ibt and -z shstk? (My understanding is that they should be very similar to -z force-bti and -z pac-plt, respectively.)

For MPX prefix:
GCC have not supported the MPX from GCC 9. And intel will not support MPX code too. So we don’t consider MPX for CET in LLD.

I know that GCC has removed MPX and the Linux kernel is removing MPX (user-visible APIs and self-tests have been removed). I asked because I haven't seen a change on binutils-gdb side that will support a .plt.sec scheme without the BND prefix. So, I wonder what kind of changes are considered divergence from x86-64 psABI. After the removal of the BND prefix, the .plt entry will get the leeway of 2 bytes. If, say, in the future, a new security enhanced feature is proposed which requires a new instruction that will take more than 2 bytes, the 16-byte .plt entry no longer works, and toolchains will have to migrate a third PLT scheme, different from traditional PLT and the .plt.sec scheme.

As to the option name question, are you happy with -z force-ibt and -z shstk? (My understanding is that they should be very similar to -z force-bti and -z pac-plt, respectively.)

These options are OK for us, thank you again!
By the way, as I know, gnu ld have no CET options, (excepted -z cet-report=xxx you point out before). it just try to add CET flags if all input file CETed.
For MPX prefix, in my eyes, it seems the new Binutils just forget to remove it. Try ld --help you can see it list "-z nobndplt Generate a regular PLT (default)"

MaskRay updated this revision to Diff 233731.Dec 12 2019, 8:38 PM
MaskRay edited the summary of this revision. (Show Details)

Delete --force-cet. Add -z force-ibt and -z shstk.

For MPX prefix:
GCC have not supported the MPX from GCC 9. And intel will not support MPX code too. So we don’t consider MPX for CET in LLD.

I know that GCC has removed MPX and the Linux kernel is removing MPX (user-visible APIs and self-tests have been removed). I asked because I haven't seen a change on binutils-gdb side that will support a .plt.sec scheme without the BND prefix. So, I wonder what kind of changes are considered divergence from x86-64 psABI. After the removal of the BND prefix, the .plt entry will get the leeway of 2 bytes. If, say, in the future, a new security enhanced feature is proposed which requires a new instruction that will take more than 2 bytes, the 16-byte .plt entry no longer works, and toolchains will have to migrate a third PLT scheme, different from traditional PLT and the .plt.sec scheme.

As to the option name question, are you happy with -z force-ibt and -z shstk? (My understanding is that they should be very similar to -z force-bti and -z pac-plt, respectively.)

These options are OK for us, thank you again!
By the way, as I know, gnu ld have no CET options, (excepted -z cet-report=xxx you point out before). it just try to add CET flags if all input file CETed.
For MPX prefix, in my eyes, it seems the new Binutils just forget to remove it. Try ld --help you can see it list "-z nobndplt Generate a regular PLT (default)"

-z nobndplt is not implemented in GNU ld

% ld/ld-new -z nobndplt -v
./ld-new: warning: -z nobndplt ignored
GNU ld (GNU Binutils) 2.33.50.20191213

gold supports -z nobndplt but it does not implement IBT.

% gold/ld-new --help | grep bndplt
  -z bndplt                   (x86-64 only) Generate a BND PLT for Intel MPX
  -z nobndplt                 Generate a regular PLT (default)

I have uploaded a diff that implements -z force-ibt and -z shstk, but ultimately it is the code owner (@ruiu)'s decision whether the change should be included.

@ruiu,
Can we submit the patch?

PIng.
@ruiu LLVM 10.0.0 will be branched on Jan. 15. I would expect that the LLD CET support can be included in this release. If you have concern with current implementation, let's have an offline discussion and move it forward quickly.

@ruiu If you are ok with this, I'll rebase, fix the PLT declarations (since I updated them in previous refactorings) and commit.

ruiu added a comment.Jan 9 2020, 6:44 AM

@MaskRay Please rebase and upload it with me and other people as reviewers, then I'll LGTM.

MaskRay commandeered this revision.Jan 9 2020, 10:27 AM
MaskRay edited reviewers, added: ruiu; removed: MaskRay.
This revision now requires review to proceed.Jan 9 2020, 10:27 AM
This comment was removed by MaskRay.
MaskRay updated this revision to Diff 237157.EditedJan 9 2020, 12:19 PM

@ruiu Done.

Rebase on top of D72474

iplt works. Tested several combinations

clang -m32 -static -fcf-protection=full a.c -fuse-ld=lld -Wl,-z,force-ibt -fno-PIC -o a
clang -m64 -static -fcf-protection=full a.c -fuse-ld=lld -Wl,-z,force-ibt -fno-PIC -o a
clang -m32 -static -fcf-protection=full a.c -fuse-ld=lld -Wl,-z,force-ibt -fPIC -o a
clang -m64 -static -fcf-protection=full a.c -fuse-ld=lld -Wl,-z,force-ibt -fPIC -o a
clang -m32 -fcf-protection=full a.c -fuse-ld=lld -Wl,-z,force-ibt -fPIC -o a
clang -m64 -fcf-protection=full a.c -fuse-ld=lld -Wl,-z,force-ibt -fPIC -o a

Unit tests: fail. 61724 tests passed, 1 failed and 779 were skipped.

failed: lld.ELF/x86-64-feature-cet.s

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay updated this revision to Diff 237175.Jan 9 2020, 1:01 PM

Fix manpage

Unit tests: fail. 61725 tests passed, 1 failed and 779 were skipped.

failed: lld.ELF/x86-64-feature-cet.s

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

xiangzhangllvm added inline comments.Jan 9 2020, 6:38 PM
lld/test/ELF/x86-64-feature-cet.s
41

lit test failed here, Here we mainly check is the first plt address is written correct in .got.plt
and we can really see the data (50132000)
201350: f3 0f 1e fa endbr64 // First PLT

I see here (line 87) add indirect function check, do we need to deal with this special function in this patch?
or deal with it in another independent patch?

MaskRay marked an inline comment as done.
MaskRay added inline comments.
lld/test/ELF/x86-64-feature-cet.s
41

On RELA targets, .got.plt slots for IFUNC are not used. They can be kept zero. See D72474

Unfortunately pre-merge-bot does not recognize patch dependencies, so x86-64-feature-cet.s will fail.

xiangzhangllvm added inline comments.Jan 10 2020, 3:11 AM
lld/test/ELF/x86-64-feature-cet.s
41

So can we remove the GOTPLT check here let it 'skip' the bot check? because LLVM 10.0.0 will release on Jan 15, 2020. We can add back the GOTPLT check after the dependent patch merged.

MaskRay marked 2 inline comments as done.Jan 10 2020, 10:37 AM
MaskRay added inline comments.
lld/test/ELF/x86-64-feature-cet.s
41

You did not need to worry about the test...

Anyway I've committed the .got.plt change. This patch should pass all tests now.

@ruiu Good to merge?

MaskRay updated this revision to Diff 237391.Jan 10 2020, 10:38 AM
MaskRay marked an inline comment as done.

Rebase after .got.plt change (D72474).

merge_guards_bot should be happy now.

Unit tests: pass. 61763 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

ping @ruiu Hi, Do you have time to review? Thanks!

@MaskRay Please rebase and upload it with me and other people as reviewers, then I'll LGTM.

@MaskRay as @ruiu said above, I think you may assume that the patch is LGTH after it's rebased, if he has no other comments today. Given that, can you merge the patch in before LLVM branching?

This revision was not accepted when it landed; it landed in state Needs Review.Jan 13 2020, 11:48 PM
This revision was automatically updated to reflect the committed changes.

Thank you again!!