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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

> deficiencies：
> 
> 1. I tend to collect the CET features in GnuPropertySection (SyntheticSection) class or in Writer->run like combineEhFrameSections(), but I found it should first get the CET features, then decide to create the GnuPropertySection or not (SPltSection too).
> 2. I didn't remove the Config->X86Feature1AND, because I need it to tell the createSyntheticSections() to create the GnuPropertySection or not (SPltSection too).

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.



================
Comment at: ELF/SyntheticSections.cpp:300
+
+  // To Be Dnone
+  // Now we only supported X86_FEATURE_1_AND in .note.gnu.property, so we fix
----------------
Typo Dnone, should be Done. Having said that. You have already commented that only X86_FEATURE_1_AND is supported, do you need to repeat it? 


================
Comment at: ELF/SyntheticSections.h:154
+  // To Be Dnone
+  // Now we only supported X86_FEATURE_1_AND in .note.gnu.property, so we fix
+  // the DescSize to one Desciption size, it may be mare than one Desciption
----------------
The comment has quite a few typos. May I suggest something like:
// We only support a single feature X86_FEATURE_1_AND, so we hard code the DescSize to a single entry.


================
Comment at: ELF/SyntheticSections.h:157
+  // when we try to fix other feature types in .note.gnu.property.
+  size_t getDescSize() {return 16;}
+  void setDescSize();
----------------
This doesn't need to be a function, you only use it once to initialize DescSize. Why not just make DescSize static const size_t DescSize = 16; 


================
Comment at: ELF/SyntheticSections.h:158
+  size_t getDescSize() {return 16;}
+  void setDescSize();
+  uint32_t getFeature();
----------------
These member functions are only called once and can be inlined.


================
Comment at: ELF/SyntheticSections.h:161
+  void setFeature();
+
+private:
----------------
If you implement the empty() member function then you will be able to unconditionally create In.GnuProperty as it will be removed if it isn't  used.


================
Comment at: ELF/SyntheticSections.h:164
+  size_t DescSize;
+  uint8_t *DescBuf;
+  uint32_t Feature1AND;
----------------
DescBuf is only used in writeTo, it can be defined there. 


================
Comment at: ELF/Writer.cpp:88
+
+  // The Writer will deal with the InputSection's data array which is just a
+  // point in class ArrayRef, so the caller should initialize the data array
----------------
You aren't using this anymore so it can be removed.


================
Comment at: ELF/Writer.cpp:424
+
+  if (Config->X86Feature1AND) {
+    In.GnuProperty = make<GnuPropertySection>();
----------------
If you implement the GnuPropertySection::empty() member function then you can unconditionally create the section. It will be removed by removeUnusedSynthetics() if there are no entries.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D58102