theraven marked 2 inline comments as done.
theraven added inline comments.
================
Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188
+ StringRef SymbolPrefix() {
+ return CGM.getTriple().isOSBinFormatCOFF() ? "_" : "._";
----------------
DHowett-MSFT wrote:
> Should this be `SymbolPrefix()` or something more like
> `MangleSymbol(StringRef sym)`? I know that right now we only need to prepend
> `_` or `._`, but is it more future-proof to make it generic?
I have refactored this, and then tried adding a $ instead of the . for
mangling. On further testing, the latest link.exe from VS 2017 no longer
complains about symbols starting with a dot, so I'm inclined to revert that
part of it entirely (lld-link.exe was always happy). I'd prefer to minimise
differences between COFF and ELF and this means that we have different section
names, but aside from needing the extra global initialisation on COFF,
everything else is the same.
================
Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:1528
InitStructBuilder.addInt(Int64Ty, 0);
- for (auto *s : SectionsBaseNames) {
- auto bounds = GetSectionBounds(s);
- InitStructBuilder.add(bounds.first);
- InitStructBuilder.add(bounds.second);
- };
+ if (CGM.getTriple().isOSBinFormatCOFF()) {
+ for (auto *s : PECOFFSectionsBaseNames) {
----------------
DHowett-MSFT wrote:
> We may be able to reduce the duplication here (aside: it is very cool that
> Phabricator shows duplicated lines) by capturing `auto sectionVec =
> &SectionsBaseNames;` and switching which is in use based on bin format.
I thought about that and didn't do it because if the two arrays are different
lengths, they're different types. But, of course, they're the same length and
if they're ever different lengths in the future then that's probably a bug.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58724/new/
https://reviews.llvm.org/D58724
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
