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

Tue Mar 12 10:59:47 PDT 2019

ruiu added a comment.

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

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

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.

>> I think there are still several issues with this patch. I pointed out some of them before, but to summarize, let me iterate:
>> 
>> - Your patch collects `X86Feature1` bits from input files to copy that to a .note section in the output file. I don't think you need to do that. You only need to make sure that the input files have a desired bit in `X86Feature1` bits, and generate a .note section containing `GNU_PROPERTY_X86_FEATURE_1_IBT` bit. In other words, please remove `X86Feature1AND` from `Config`.
> 
> 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?
> 
> 1. If not all input files have `X86Feature1` bits, then discard the .note.gnu.property sections.
> 2. If all input files have `X86Feature1` bit**s**: then we need to 'calculate' the resulting `X86Feature1` bit**s**  from all the  `X86Feature1` bits ?           (yes or no ?) then delete all the input files' .note.gnu.property sections ?                                                                       (yes or no ?) then generate a .note.gnu.property section with the resulting `X86Feature1` bits  in input file level not in output file by Writer, so we need not to pass the `X86Feature1` bits to Writer ?                                                                          (yes or no ?)

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.

>> - You add fair amount of new code to Driver.cpp, but which feels wrong. It should not be part of a driver. But I think the real problem is that the code to read .note section does just too much.
> 
> Hi, ruiu, there are not very much code now, in fact, half of them are code annotation.
> 
>> - Do you really have to read a .note section containing multiple descriptions? I think that compiler-created .note section contains only one description per a section, so you don't need to care about it.
> 
> Yes, the compiler-created .note section contains only one description per a section, But we should consider the handwriting assemble files.

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

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

https://reviews.llvm.org/D58102