Thank you again!!

LGTM

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

update the patch, and I tested in my local performance test, it really have a little small improvement.

In D59780#1782133, @MaskRay wrote:

In D59780#1781055, @xiangzhangllvm wrote:

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.

In D59780#1778988, @MaskRay wrote:

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.

Remove the "-plugin xxx", also work.

In D59780#1778672, @MaskRay wrote:

In D59780#1778623, @xiangzhangllvm wrote:

In D59780#1738474, @ruiu wrote:

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.

In D59780#1738474, @ruiu wrote:

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

In D70157#1746793, @MaskRay wrote:

On x86, the preferred function alignment is 16 (https://github.com/llvm/llvm-project/blob/arcpatch-D70157/llvm/lib/Target/X86/X86ISelLowering.cpp#L1893), which is the default function alignment in text sections. If the cross-boundary decision is made with alignment=32 (--x86-align-branch-boundary=32) in mind, and the section alignment is still 16 (not increased to 32 or higher), the linker may place the section at an address which equals 16 modulo 32, the section contents will thus shift by 16. The instructions that do not cross the boundary in the object files may cross the boundary in the linker output. Have you considered increasing the section alignment to 32?

Shall we default to -mbranches-within-32B-boundaries if the specified -march= or -mtune= may be affected by the erratum?

Done again, thank you again!!

All changed, Thank you very much!

Done, Thank you very much!

Hi Dear friends, could you help merge this patch? Thank you very much!

Hi Dear friends, could you help merge this patch? Thank you very much!

rebase

rebase

In D59780#1520411, @MaskRay wrote:

In D59780#1520390, @xiangzhangllvm wrote:

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.

rebase

rebase

Rename tests in test/CodeGen/X86

rebase

rebase

Thank You very much! Fangrui

Hi friends, I write a simple test to test the performance:
To simply test the performance affect I changed the PLT size from 16 Bytes to 24Bytes in LLVM 8.0.0.

[xiangzh1@scels74 /export/iusers/xiangzh1/LLVM/LLVMORG]$diff llvm800/tools/lld/ELF/Arch/X86_64.cpp llvm800-PLT24/tools/lld/ELF/Arch/X86_64.cpp
67c67
<   PltEntrySize = 16;
---
>   PltEntrySize = 24; //16-->24
157a158,159
>       0x66, 0x90,                   //nop
>       0x66, 0x0f, 0x1f, 0x44, 0, 0, // nop
[xiangzh1@scels74 /export/iusers/xiangzh1/LLVM/LLVMORG]$

Then I write a simple case like following:

We are Ok with with the 2-PLT scheme.

Hi Ruiu & Fangrui
Your idea is also make sense, but It is really too too late to change ABI now.
I wish we can reach a consensus as soon as possible.
Thank you all, very much!

Hi MaskRay, I tested your idea by changing the patch:

In D59780#1442757, @MaskRay wrote:

So on the compiler driver side, -fcf-protection is the single option end users are concerned. I believe this option hasn't been assigned linker option semantics in either GCC or clang. If assigning it the linker option semantics is favorable, we should make it compatible in GCC/clang and ideally use the same options in lld/ld.bfd/gold.

The current form of this patch just implements --force-cet, thus I believe the use cases are:

gcc a.o -o a # create `.splt` (or `.plt.sec`) if all of a.o and other stdlib have IBT bit set
             # I worry that someone may not want .splt for this case
  ld.lld a.o ... -o a

If all the files contain IBT, it means all the files compiled with -fcf-protection(=branch) ,there is no reason for user not to enable the IBT.

In D59780#1442728, @MaskRay wrote:

Is --force-cet the best option name? H. J. Lu pointed out that gold has -z ibtplt and other options, but I couldn't find these options in the binutils' repository. Is the option really implemneted to gold?

The binutils-gdb repository hosts two linkers: ld/ld-new (GNU ld) and gold/ld-new (GNU gold). -z ibtplt and -z ibt are ld.bfd options, not gold's. I asked in https://reviews.llvm.org/D58102 if there is a proposal to add support to gold but get no response so far...

I've done some experiments and let me clarify things. @xiangzhangllvm Please correct me I missed something.

1. On the compiler side
   
   Support for Intel CET requires GCC 8 (https://gcc.gnu.org/gcc-8/changes.html).
   
   -fcf-protection= is used to define __CET__ (branch: 1, return: 2, full: 3) and emit .note.gnu.property in object files.
   
   In some older releases of GCC 8 (before Apr 20, 2018), CET requires two other options -mibt and -mshstk, which enable the macros __IBT__ __SHSTK__, respectively. -mcet implies both -mibt and -mshstk and defines __CET__. -mshstk enables several builtin functions.
   
   -mibt is deleted in some newer GCC 8 release. -mshstk controls only the availability of some builtin options. If you are using some older releases of GCC 8:
   
   ` gcc a.c -mibt -c # __IBT__ is defined but no .note.gnu.property gcc a.c -fcf-protection=branch -mibt -c # NT_GNU_PROPERTY_X86_FEATURE_IBT tag gcc a.c -fcf-protection=return -mshstk -c # NT_GNU_PROPERTY_X86_FEATURE_SHSTK tag gcc a.c -fcf-protection=full -mcet -c # -mcet is short for -mibt -mshstk, IBT + SHSTK `
   
   In newer GCC, forget about -mibt -mshstk and just use -fcf-protection=. All these options do no affect codegen.
2. On the linker side
   
   ibt is what concerns linkers. shstk has no influence other than the output note section. The feature is only implemented in ld.bfd, not in gold.
   
   -z ibtplt generates the second PLT .plt.sec (not .splt). -z ibt implies -z ibtplt. The option does not check the input NT_GNU_PROPERTY_X86_FEATURE_IBT tag.
   
   -z ibt emits NT_GNU_PROPERTY_X86_FEATURE_IBT but plain -z ibtplt doesn't.
   
   GCC 5~8 support Intel MPX and you shall see the bnd prefix in some jump instructions. Note that GCC 9/Linux kernel 4.18 drop MPX. Fortunately it seems MPX doesn't change the size of PLT entries. Thus in lld we don't have to support MPX, but we are still flexible enough to add related support if it revives in the future.

Is --force-cet the best option name? H. J. Lu pointed out that gold has -z ibtplt and other options, but I couldn't find these options in the binutils' repository. Is the option really implemneted to gold?
What is the compiler flag to enable CET? I'd like to write it in a comment for reference.

Hi, Ruiu, You may need update ld to 2.31+ version:
xiangzh1@xiangzh1:~$ ld -v
GNU ld (GNU Binutils) 2.32
xiangzh1@xiangzh1:~$ ld --help
elf_x86_64:

 ...
-z ibtplt                   Generate IBT-enabled PLT entries
-z ibt                      Generate GNU_PROPERTY_X86_FEATURE_1_IBT
-z shstk                    Generate GNU_PROPERTY_X86_FEATURE_1_SHSTK

All these options in gold is just for test.

Update test to check splt's context

rebase

The update:

Hi peter, this update will remove redundant .splt in your upper ifunc test.
The reason of redundant .splt is that we scan the IFUNC symbol after we collected the CET feature. So the splt was created first and without being removed after we found IFUNC.
Anyway this patch is just evade ifunc, I planed to hand ifunc+cet after this patch.
Thank your test! thank you too

Bug fix for 32-bit, the Desc size is not same between 32-bit and 64-bit

In 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(), .........

In D58102#1436114, @peter.smith wrote:

I'm having problems getting this to compile on at least GCC 5.4.0 on Linux:

lld/ELF/SyntheticSections.cpp:3104:28: warning: ISO C++ forbids zero-size array ‘Desc’ [-Wpedantic]
   GnuPropertyDescType Desc[]

lld/ELF/SyntheticSections.cpp:3110:55: error: invalid conversion from ‘llvm::cast_retty<GnuPropertyType, const unsigned int*>::ret_type {aka const GnuPropertyType*}’ to ‘GnuPropertyType*’ [-fpermissive]
   GnuPropertyType *GnuProperty = cast<GnuPropertyType>(Data.data());

The cast<> is an LLVM cast http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates and isn't something you can do on an arbitrary struct.

Assuming (x86) that your input data is always little endian then you'll need to use something like ulittle32_t instead of uint32_t or your tests will fail when run on a big-endian machine like PPC or some Mips machines.

I rewrited the collecting feature functions.
It is very simple now!

Fine tuning the patch, and still need to refine the "code style".

In D58102#1435024, @ruiu wrote:

Overall this patch still look very different from the existing lld code base and does many things by hand without using LLVM libObject. You really need to read the existing code first and then try to mimic what the existing code is doing. What this patch is trying to do is not that new -- your patch should and could follow the existing patterns that I believe not too hard to find by reading existing code.

It is perhaps faster to actually show how it should look like than pointing out all the details in the review thread, so I'll apply your patch locally, modify that, and then upload it so that you can read it.

In D58102#1434642, @peter.smith wrote:

As I understand it, what you have in this review (not the patch attached as a comment).

• Driver.cpp calls mergeAggregateMetaData() to set the Config->SpltSectionEnabled and Config->X86Feature1AND
  • We know that all input objects have the feature property set.
• Writer.cpp creates the In.SpltSection() and the In.GnuProperty() section as we know that will be needed.
• If we encounter an ifunc then we disable IBT by setting Config->SpltSectionEnabled to false and clearing Config->X86Feature1AND
  
  One thing I don't understand yet, apologies I've not had time to go into the details of how the splt section works, is that if you discover an ifunc mid-way through processing then you'll have inserted many entries into the In.Splt section, then disabled it, from then on you'll just use the In.Plt. Will this result in a broken PLT as it looks like we'll call get getPltVA() if Config->SpltSectionEnabled and I'm not sure if that will return the right thing if we've previously added an entry into the In.Splt section. I don't see any existing tests for ifunc handling, can you add a test case that does something like:
• Enable SpltSection
• Generate a In.SpltSection entry
• encounter an Ifunc
• Disable SpltSection
• Generate a In.Plt entry for some other symbol
• Check that the generated PLT makes sense.
  
  If this bit is broken then I think you'll need to find a way of handling this case, which won't be easy. I can think of a few approaches:
• Do a pre-scan for likely ifuncs, before calling scanRelocs. This could be simpler but more pessimistic than the existing code.
• Have a way that the In.SpltSection decays into an In.PltSection gracefully if an ifunc is detected.
  
  Going back to your original question, how about:
• Writer.cpp at construction of In.GnuPropertySection calls mergeAggregateMetaData() and sets GnuPropertySection::Feature1AND and possibly SpltSection::enable
  • As you've implemented empty() these sections can be unconditionally created, although it doesn't matter too much if you conditionally create them, in that case In.GnuPltSection == nullptr is implicity Feature1AND=0 and In.SpltSection == nullptr is implicitly enableSpltSection == false
• If we encounter an ifunc then we disable IBT by setting In.SpltSection::Enable to false and setting In.GnuPropertySection::X86Feature1AND to 0. If you've not created the sections then you'll obviously need to test that they exist first. If I've understood correctly if the sections don't exist then there is nothing to disable anyway.
  
  Please let me know if I've misunderstood something critical?

I do this changes in my local:
I do the feature collection work in the GnuPropertySection's Constructor which will be called in createSyntheticSections().
For the SPlt, I keep its generation condition, because the GnuPropertySection's Constructor will set the Config->SPltSectionEnable.

In D58102#1433093, @peter.smith wrote:

I guess you mean "I tried", not "I tend" in 1). I think that you can do accomplish 1.) via unconditionally creating the In.GnuPropertySection and SPltSection and by implementing the empty() member function, ensure that they are removed by removeUnusedSyntheticSections() if there isn't anything that uses them. Unconditionally creating the sections means that you don't need to call mergeAggregateMetaData from Driver.cpp prior to createSyntheticSections(). For the case of ifunc where you need to turn off the feature then just inform In.GnuPropertySection and In.SPltSection as they will always be there.

In D58102#1433556, @emaste wrote:

In D58102#1422441, @hjl.tools wrote:

We are using a special CET SDV for CET enabling. We have been enabling CET in Fedora piece by piece by adding -fcf-protection to CFLAGS.

How do we get access to such a simulator so that we can do the same in FreeBSD?

In D58102#1428198, @ruiu wrote:

But gold already has options, -z ibt and -z ibtplt. You should be consistent with these options, shouldn't we?

Yes, I think we should, and I think it's better to support these test options in a independent patch, and it's easy to support.

hi dears,
I updated the patch: most of changes refer to peter and ruiu's suggestions, but not of all.
The main change is:

1. Read all input files, then collect the CET features, and discard the .note.gnu.property sections, do these in SyntheticSections.cpp not in Driver.cpp.
2. If CET features checked successful, then create the SyntheticSection's sections (GnuPropertySection, SPltSection) in Writer's createSyntheticSections().

instead of directly creating .note.gnu.property section in Writer.

3. Adjust the test cases to remove redundant code check.