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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm marked 7 inline comments as done.
xiangzhangllvm added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:300
+
+  // To Be Dnone
+  // Now we only supported X86_FEATURE_1_AND in .note.gnu.property, so we fix
----------------
peter.smith wrote:
> 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? 
Yes, sorry for the typo, thank you!
I repeat the reason here is to let people easily understand and they will know how to change it when the other feature added.


================
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
----------------
peter.smith wrote:
> 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.
Sorry for the typos, I 'll change it, thank you!


================
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();
----------------
peter.smith wrote:
> 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; 
For consideration of extensibility, I used function to get the DescSize. We will do some work here when we need deal with the other feature types, because there will create more than one Desc here.


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


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


================
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
----------------
peter.smith wrote:
> You aren't using this anymore so it can be removed.
Yes, you are right!


================
Comment at: ELF/Writer.cpp:424
+
+  if (Config->X86Feature1AND) {
+    In.GnuProperty = make<GnuPropertySection>();
----------------
peter.smith wrote:
> 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.
Thank you! I want to try.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D58102