[PATCH] D101395: [lld-macho] Implement builtin section renaming

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Yeah keeping `-data_const` makes sense. Thanks for removing the rest :)



================
Comment at: lld/MachO/Driver.cpp:560-561
 
+// LD64 gives precedence to command-line renames via -rename_section and
+// -rename_segment
+
----------------
Our implementation does so too, right? Would be good to make that explicit... also I think this comment would make more sense at the point where we invoke `initializeSectionRenameMap` to highlight that we are calling it before handling OPT_rename_section


================
Comment at: lld/MachO/Driver.cpp:566-572
+         {section_names::got, section_names::authGot, section_names::authPtr,
+          section_names::nonLazySymbolPtr, section_names::const_,
+          section_names::cfString, section_names::moduleInitFunc,
+          section_names::moduleTermFunc, section_names::objcClassList,
+          section_names::objcNonLazyClassList, section_names::objcCatList,
+          section_names::objcNonLazyCatList, section_names::objcProtoList,
+          section_names::objcImageInfo})
----------------
nit: I think this would be cleaner as a separately-declared array


================
Comment at: lld/MachO/Driver.cpp:577-579
+  // Note: ld64 handles all renames above this line textually.  For
+  // renames below this line, ld64 reassigns internal section-id enums.
+  // We will endeavor to do them all textually
----------------
Do we really need to discuss ld64's implementation details here?


================
Comment at: lld/MachO/Driver.cpp:815
+  case MH_OBJECT:
+  case MH_PRELOAD:
+    return false;
----------------
I think we're unlikely to support `-preload` any time soon either


================
Comment at: lld/MachO/Driver.cpp:818
+  default:
+    llvm_unreachable("output type to determine data-const");
+  }
----------------
maybe "unsupported output type when trying to determine data-const"?


================
Comment at: lld/MachO/MergedOutputSection.cpp:26-27
   } else {
-    mergeFlags(input->flags);
     align = std::max(align, input->align);
+    mergeFlags(input);
   }
----------------
intentional change?


================
Comment at: lld/MachO/MergedOutputSection.cpp:59-61
+    error("Cannot add merge section; inconsistent type: old=0x" +
+          llvm::to_hexString(SECTION_TYPE & flags) + ", new=0x" +
+          llvm::to_hexString(sectionFlag));
----------------
gkm wrote:
> gkm wrote:
> > This has no test. It feels like overkill to make a testcase for it. This is a bug fix discovered during testing section renames. The old message appeared as ...
> > ```
> > Cannot add merge section; inconsistent type flags ^@
> > ```
> > Where the section type was a raw NUL (0) byte, and clearly wrong.
> After adding section names to the diagnostic message, I went ahead and added a test case. ;)
Thanks! I think it's good to test error messages, particularly since they're infrequently executed on typical inputs. (The fact that you discovered a latent bug further demonstrates this :) )

nit: no need for `llvm::`


================
Comment at: lld/test/MachO/builtin-rename.s:5
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin \
+# RUN:     %t/main.s -o %t/main.o 2>/dev/null
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin \
----------------
why the `2>/dev/null`?


================
Comment at: lld/test/MachO/builtin-rename.s:25-27
+# RUN:     Filecheck %s --check-prefixes=CHECK,NDATA
+# RUN: llvm-objdump --syms %t/nopie | \
+# RUN:     Filecheck %s --check-prefixes=CHECK,NDATA
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101395