[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

[David Chisnall via Phabricator via cfe-commits]

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