[PATCH] D101395: [lld-macho] Implement builtin section renaming
Jez Ng via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 28 02:17:14 PDT 2021
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
More information about the llvm-commits
mailing list