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

Wed Mar 13 13:56:13 PDT 2019

ruiu added a comment.

In D58102#1427069 <https://reviews.llvm.org/D58102#1427069>, @xiangzhangllvm wrote:

> In D58102#1426542 <https://reviews.llvm.org/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).

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

What's going to happen if you specify these options to gold and some input files lack special marker `.note` section? Does the gold linker silently create an executable that won't work, or does it reject with an error message?

>> 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.

Fair enough. Maybe the thing that alerted me on that code was that the code was written in a very different way than the other code in the same .cpp file and other .cpp files in the same directory. First of all, the code to read that section should not be in the driver, and that was written in a different coding style. Perhaps after you fix that (probably you need to read code around the location where you are editing and mimic the way what they are doing), the code would blend into other pieces of code.

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

https://reviews.llvm.org/D58102