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.

In D56990#1426977, @efriedma wrote:

LGTM; I'll merge it tonight or tomorrow.

In D58102#1426542, @ruiu wrote:

Please keep in mind that "let's think about that later" is not a correct approach for command line option names. That's a public interface that cannot be changed once released. I think that is also true to gold. Even if the intention to add the -z options to gold was for testing, if the options have been added in a user-visible way, it is very unlikely that they'd be removed. So you need to think carefully as to what is the most consistent set of option names both for gold and lld once you implement all the features you want. That the situation that lld has --{auto,force}-cet and gold has -z options for the same functionality seems pretty inconsistent. Probably the importance of option names should be taken more seriously.

lld's --{auto,force}-cet is different with gold's -z options, "-z" options directly add CET info into output files, not care the input files is CETed or not, they just designed for testing, it is not right to use them in normal build.
--{auto,force}-cet depend on the input files to enable CET or not.
The key difference is that gold auto enabled CET in default (equal with LLD's --auto-cet).
I tended to keep it same with gold, but seems hard to persuade you ☺

What I recommended is to always discard .note section after checking the GNU_PROPERTY_X86_FEATURE_1_IBT *bit*. There's no .note section merging or anything. When you parse a file, just read a single bit from a .note section (if exists), and discard it.
As a result, after you read all input files, you have only one bit -- which indicates whether or not all input files support CET.
Then, if CET is supported by all input files, create a synthetic section for a .note section to enable CET for the output.

OK, as peter's suggestion, I'll recheck here code, thank you！

I'm skeptical if people actually hand-write a .note section in assembly. In particular, is it really normal or even correct to write a note section containing multiple descriptions in assembly by hand?

The assemble syntax support multiple descriptions.
And suppose that, one day, not only CET feature will put into .note.gnu.property, (now ABI have defined several other kind features such as GNU_PROPERTY_X86_ISA_1_USED ... ). So I think it will be normal to see one section with multiple descriptions (each description contains a different feature).

When you enable the CET for an assembly file, you would pass a command line option to the assembler to let the assembler to add that section, no? I seems that you are overthinking. I'd just consider the case where there is only one note description in a .note section.

Yes, no, we write the .note.gnu.property in the assembly file not use options.

In D58102#1426422, @peter.smith wrote:

As an earlier comment mentioned, there is quite a lot of code that depends on the details of the .note.gnu.property section added very early to Driver.c which is not where we'd normally expect to see it. It also looks like you using quite a lot of code to get an InputSection to represent the output .note.gnu.property section. Given that there will be multiple Input .note.gnu.property sections but only one Output .note.gnu.property section, I think it would be cleaner to use a SyntheticSection to represent the .note.gnu.property section. SyntheticSections can implement all sorts of custom functionality that could move the parsing and merging code out of Driver.cpp. My suggestion is to:

• Create a new SyntheticSection (see SyntheticSections.h), something like NotePropertySyntheticSection. This will represent our single output .note.gnu.property section. It can store the status of the FEATUREAND bits.
• Add an entry to InStruct for a single instance of it that we will use for the output.
• After InStruct instance is created in Writer.c but before the relocations are scanned to create PLT entries iterate through InputSections and pass each .note.gnu.property to the NotePropertySyntheticSection. Then erase these InputSections from the InputSections. The combineEhFrameSections<ELFT> function in Writer.cpp is one way of doing something like that.
• When you add the .note.gnu.property InputSection to the NotePropertySyntheticSection you can update the FEATUREAND bits
• The writeTo() member function of the NotePropertySyntheticSection can output the data needed to make up the output.
  
  I think that this should allow you to integrate the .note.gnu.property in a similar way to other "merge" features such as EHFrame, and SHF_MERGE sections.
  
  The ld.bfd AArch64 BTI patches have just gone up on https://www.sourceware.org/ml/binutils/2019-03/msg00021.html so I'll hopefully be starting work on another feature that uses .note.gnu.property soon.

We found may tests failed about this issue.
I hope It can be committed.
Thank you very much!

static void setConfigs(opt::InputArgList &Args);

In D58102#1425580, @MaskRay wrote:

Gold auto enabled CET in default

What kind of CET support does gold have now? I think it can merge .note.gnu.property now but I don't find code related to second PLT or ibtplt. (I have a question about ibtplt, see below).

I add --auto-cet and --force-cet options, and disable CET in default. Now there will be no influence to the LLD when not using these options.

I find that ld.bfd has three related options -z ibt -z ibtplt -z shstk (are there others?). What do they do?

-z ibtplt                   Generate IBT-enabled PLT entries\n"));
fprintf (file, _("\
-z ibt                      Generate GNU_PROPERTY_X86_FEATURE_1_IBT\n"));
fprintf (file, _("\
-z shstk                    Generate GNU_PROPERTY_X86_FEATURE_1_SHSTK\n"));

Is -z ibtplt similar to the second PLT? Does -z ibt generate GNU_PROPERTY_X86_FEATURE_1_IBT even if there is an input object file with the GNU_PROPERTY_X86_FEATURE_1_IBT bit cleared? Or when it is specified while -z shstk is not, merge just ibt but not shstk? I'm puzzled as you said the note merging behavior is automatic in ld.bfd.

--auto-cet and --force-cet

We can also make one option --cet={none,auto,force}, but I'd like to understand first what -z ibt -z shstk -z ibtplt do...

Yes, ruiu, last time I also ask you about this point. please let me summarize your ideas here, then please check if I understand right?

In D58102#1424938, @ruiu wrote:

--auto-cet and --force-cet is perhaps fine. That should ensure users are aware of what they are doing (so the feature is not enabled too automatically), and it also provides a way to pin it so that it won't be accidentally turned off.

Was there any reason to choose that option names? H. J. Lu mentioned that GNU linkers supports several -z options for CET. If there's no strong reason to invent new option names, we should use the same command line options as GNU.

Gold auto enabled CET in default, so it have no options corresponding to the --auto-cet and --force-cet.
The '-z' options in gold is just for testing, For Simplify，It better to add them in a dependent patch.

Hi, efriedma
could you help he commit this patch?
Thank you very much!

Hi friends:
I add --auto-cet and --force-cet options, and disable CET in default. Now there will be no influence to the LLD when not using these options.
I did these because I want to quickly finish the options discussion, I think it is easy to refine in the future.
Thank you very much!

if just .note.gnu.property require 64-bit alignment, I think it time to change the alignment of this section.

Yes, MaskRay, let's get consensus on Auto vs Manual soon,
If you and @ruiu do not buy the reasones of Auto way, I tend to switch to the Manual way right now, after all, it is easy to 'refine' in future.

Thank you, efriedma
But but the LLVM and Clang are different projects, I can commit the change at one time.
I 'll update the patch for clang first.

In D58102#1422698, @peter.smith wrote:

I think https://bugs.llvm.org/show_bug.cgi?id=41000 "PT_NOTE segment not packed correctly" has some relevance to this review. In particular the .note.gnu.property sections on 64-bit machines have 8-byte alignment, merging these with 4-byte aligned note sections such as build-id can cause problems for consumers of PT_NOTE (see also https://sourceware.org/ml/libc-alpha/2018-08/msg00340.html for a discussion on the glibc mailing list). My understanding is that the eventual solution was to collate 4 and 8 byte SHT_NOTE sections separately and output a PT_NOTE section for each. I think that we should follow that and have tests for it.

Rebase

!!! Hi, dear efriedma, very sorry! I just saw your reply.
line 2093: getTargetHooks().adjustInlineAsmType(... InputConstraint,...) will just deal with the constrain string, and it can't check the TiedOperand in the function.
So, this will make inconsistent adjust for the operand and its tied operand.
The error will not be found in the IR files, it will cause back end error. like:
"error in backend: Unsupported asm: input constraint with a matching output constraint of incompatible type!"

In D58102#1407746, @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.)