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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

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.



================
Comment at: ELF/SyntheticSections.cpp:318-328
+// To Be Dnone
+// Now we only supported X86_FEATURE_1_AND in .note.gnu.property, so we just
+// get the feature of GNU_PROPERTY_X86_FEATURE_1_AND property, it should be
+// changed when we try to support other feature types in .note.gnu.property.
+uint32_t GnuPropertySection::getFeature() {
+  return Config->X86Feature1AND;
+}
----------------
These functions are too trivial.


================
Comment at: ELF/SyntheticSections.cpp:3063-3064
+
+  unsigned pr_typeIndex = 0; // Offset of pr_type from start of 'desc'
+  unsigned pr_dataIndex = 2; // Offset of pr_data from start of 'desc'
+
----------------
This does not follow the LLVM naming convention.


================
Comment at: ELF/SyntheticSections.cpp:3136
+// else return false;
+static bool findGNUPropertyX86Feature1AND(InputSectionBase *S,
+                                          unsigned &X86F1ANDDescLoca) {
----------------
Essentially, when you read some structure from a file, you should cast a byte array to a struct defined in llvm/include/llvm/Object/ELFTypes.h. You should not read data like this function does.


================
Comment at: ELF/SyntheticSections.cpp:3140
+  auto SecSize = Data.size() * 4;
+  decltype(SecSize) CurSize = 0;
+
----------------
Needless use of decltype.


================
Comment at: ELF/Writer.cpp:425-426
+  if (Config->X86Feature1AND) {
+    In.GnuProperty = make<GnuPropertySection>();
+    Add(In.GnuProperty);
+  }
----------------
I don't think you need this variable.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D58102