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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: lld/MachO/MergedOutputSection.cpp:69-70
+    error("Cannot merge section " + input->name + " (flags=0x" +
+          llvm::to_hexString(input->flags) + ") into " + name + " (flags=0x" +
+          llvm::to_hexString(flags) + "): strict flags differ");
 
----------------
ditto (remove llvm::)


================
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));
----------------
int3 wrote:
> 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::`
reminder to remove `llvm::` :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101395