[PATCH] D46653: Start support for linking object files with split stacks

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

It has too many style issue which distracted me from actually reviewing code. Can you format your patch with clang-format-diff and upload again?



================
Comment at: ELF/Arch/X86_64.cpp:474
+    &X86_64<ELF32LE>::getPrologueAdjustments() const {
+  static llvm::SmallVector<SplitStackPrologueAdjustment, 4> PAs = {
+      SplitStackPrologueAdjustment(
----------------
saugustine wrote:
> ruiu wrote:
> > We generally don't use the table-driven coding pattern that much in lld, especially when compared to other LLVM subprojects. I prefer less abstracted code. I'd write this as a (boring but simple) sequence of "if" and "else if" that calls memcmp() and memcpy() to replace a matched pattern.
> That means every single target that implements split stack also has to hand code the if-the-else routine for each replacement. That's a lot of boilerplate, but done.
Yes, it requires hand-coded if-else-if expressions, but that's in my opinion easier to read.


================
Comment at: ELF/Arch/X86_64.cpp:471
+template <>
+bool X86_64<ELF32LE>::adjustPrologueForCrossSplitStack(uint8_t *Loc,
+                                                       uint8_t *End) const {
----------------
I don't think we really need to support a split-stack for the x32 ABI because IIUC split-stack is used virtually only by Go, and Go doesn't support the x32.


================
Comment at: ELF/InputSection.cpp:808
+  llvm::sort(Functions.begin(), Functions.end(),
+             [](const Defined *LHS, const Defined *RHS) {
+               return LHS->Value < RHS->Value;
----------------
L and R


================
Comment at: ELF/InputSection.cpp:812
+
+  auto MorestackCallIter = MorestackCalls.begin();
+  for (Defined *F : Prologues) {
----------------
Generally we use shorter names for variable name and function name. `MorestackCallIter` -> `It`


================
Comment at: ELF/InputSection.cpp:817
+           (*MorestackCallIter)->Offset < F->Value)
+      MorestackCallIter++;
+    // Adjust all calls inside the function.
----------------
Please use pre-++ instead of post-++.


================
Comment at: ELF/InputSection.cpp:830
+  for (Defined *F : Prologues)
+    if (F->Value <= Offset && Offset < F->Value + F->Size) return true;
+  return false;
----------------
style


================
Comment at: ELF/InputSection.cpp:841
+                                                         uint8_t *End) {
+  if (!getFile<ELFT>()->SplitStack) return;
+  std::set<Defined *> AdjustedPrologues;
----------------
Style issue.


================
Comment at: ELF/InputSection.cpp:842
+  if (!getFile<ELFT>()->SplitStack) return;
+  std::set<Defined *> AdjustedPrologues;
+  std::vector<Relocation *> MorestackCalls;
----------------
We generally avoid `std::set` because it is an ordered map and slower than `llvm::DenseMap`. If you don't need an ordered map, please use DenseMap instead.


================
Comment at: ELF/InputSection.cpp:847
+    // Local symbols can't possibly be cross-calls.
+    if (Rel.Sym->isLocal()) continue;
+    Defined *D = dyn_cast_or_null<Defined>(Rel.Sym);
----------------
Style


================
Comment at: ELF/InputSection.cpp:851
+    // A reference to an undefined symbol was an error.
+    if (!D) continue;
+
----------------
Style issue


================
Comment at: ELF/InputSection.cpp:855
+    if (D->getName().startswith("__morestack")) {
+      if (D->getName().equals("__morestack")) MorestackCalls.push_back(&Rel);
+      continue;
----------------
Style issue.


================
Comment at: ELF/InputSection.cpp:868-869
+    // then we have to be conservative.
+    if (IS && IS->getFile<ELFT>()->SplitStack) continue;
+    if (enclosingPrologueAdjusted(Rel.Offset, AdjustedPrologues)) continue;
+    if (Defined *F = getEnclosingFunction<ELFT>(Rel.Offset)) {
----------------
Style


================
Comment at: ELF/InputSection.cpp:222
+Defined *InputSectionBase::getEnclosingFunction(uint64_t Offset) {
+  if (getFile<ELFT>() == nullptr) return nullptr;
+
----------------
ruiu wrote:
> Style issue.
Can you fix?


================
Comment at: ELF/InputSection.cpp:227
+      if (D->Section == this && D->Type == STT_FUNC)
+        if (D->Value <= Offset && Offset < D->Value + D->Size) return D;
+  return nullptr;
----------------
ruiu wrote:
> Style issue. You should use clang-format to fix format issues.
Ditto


================
Comment at: ELF/Target.h:66
+  // to do the right thing. See https://gcc.gnu.org/wiki/SplitStacks.
+  virtual bool adjustPrologueForCrossSplitStack(uint8_t *Loc,
+                                                uint8_t *End) const {
----------------
Please follow the local convention -- all other functions that fail by default are in the .cpp file and uses llvm_unreachable to report an (impossible) error.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D46653