This is an archive of the discontinued LLVM Phabricator instance.

[gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.
ClosedPublic

Authored by theraven on Feb 27 2019, 9:25 AM.

Details

Summary

Based on a patch by Dustin Howett, modified to not change the ABI for
ELF platforms.

[gnustep-objc] Use more Windows-like section names.

This also makes things more readable by PE/COFF debug tools that assume
sections fit in the first header.

Patch by Dustin Howett!

Diff Detail

Repository
rC Clang

Event Timeline

theraven created this revision.Feb 27 2019, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2019, 9:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This review is mostly so that @DHowett-MSFT can check that I didn't break his patches too badly in the cleanup and test that they still do allow building Objective-C DLLs on Windows.

This looks great, and takes up the parts of my patch that I cared about. Thank you for doing this.
My primary concern is that the patch includes my "early init" changes -- while I support it and think it's the right solution, especially where it reduces double indirection on class pointers, there may be issues left to iron out.

clang/lib/CodeGen/CGObjCGNU.cpp
188 ↗(On Diff #188570)

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?

1528 ↗(On Diff #188570)

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.

theraven marked 2 inline comments as done.Feb 28 2019, 8:10 AM
theraven added inline comments.
clang/lib/CodeGen/CGObjCGNU.cpp
188 ↗(On Diff #188570)

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.

1528 ↗(On Diff #188570)

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.

DHowett-MSFT added inline comments.Feb 28 2019, 11:07 AM
clang/lib/CodeGen/CGObjCGNU.cpp
188 ↗(On Diff #188570)

The issue I had with symbols starting with . was in .DEF files specifically.

Linking a shared object containing:

@.exp_with_dot = dllexport global i32 0, align 4

without a def file, yields:

ordinal hint RVA      name

      1    0 00012900 .exp_with_dot

However, linking the same library with a definition file:

test.def

EXPORTS
 .exp_with_dot

yields

output

test.def : fatal error LNK1242: '.exp_with_dot' is an invalid export symbol name

This still reproduces with 15.9.8, sadly. LINK version 14.16.27027.1.

theraven marked 2 inline comments as done.Feb 28 2019, 11:36 AM
theraven added inline comments.
clang/lib/CodeGen/CGObjCGNU.cpp
188 ↗(On Diff #188570)

Does it work with a $ instead of a .?

DHowett-MSFT added inline comments.Feb 28 2019, 11:58 AM
clang/lib/CodeGen/CGObjCGNU.cpp
188 ↗(On Diff #188570)

Hey, $ actually works!

EXPORTS
 $exp_with_dollar
ordinal hint RVA      name

      1    0 00012900 $exp_with_dollar
DHowett-MSFT added inline comments.Feb 28 2019, 11:54 PM
clang/lib/CodeGen/CGObjCGNU.cpp
1231 ↗(On Diff #188570)

After we get the ObjCInterfaceDecl, we have to make sure it's the one corresponding to the @interface and not one for a forward declaration (@class).

+        // The first Interface we find may be a @class,
+        // which should only be treated as the source of
+        // truth in the absence of a true declaration.
+        const ObjCInterfaceDecl *OIDDef = OID->getDefinition();
+        if (OIDDef != nullptr)
+          OID = OIDDef;
+

Failure to do so can result in us either failing to import the CLASS_REF or, if it has been exported as CONSTANT, attempt a method dispatch against a pointer to our own import table.

theraven updated this revision to Diff 188885.Mar 1 2019, 4:59 AM
theraven marked an inline comment as done.
  • Address Dustin's review comments.
  • [objc-gnustep] Use $ in symbol names on Windows.
  • Add fix from Dustin.
DHowett-MSFT added inline comments.Mar 1 2019, 10:18 AM
clang/lib/CodeGen/CGObjCGNU.cpp
188 ↗(On Diff #188885)

As of the latest revision, this now fails at runtime:

0x01342976 (0x03D8D530 0x03D8DCA0 0x04045A08 0x04045A08), llvm::Twine::str() + 0x166 bytes(s), e:\src\llvm\lib\suppor
t\twine.cpp, line 29 + 0x5F byte(s)
0x01664F99 (0x03D8D5C4 0x0000000A 0x00000000 0x03D8DCA0), `anonymous namespace'::CGObjCGNUstep2::GetClassVar() + 0xB9
 bytes(s), e:\src\llvm\tools\clang\lib\codegen\cgobjcgnu.cpp, line 1207 + 0x10 byte(s)

I believe we're running afoul of StringRef's lifetime here. I haven't had a chance to dig in.

DHowett-MSFT added inline comments.Mar 1 2019, 12:35 PM
clang/lib/CodeGen/CGObjCGNU.cpp
188 ↗(On Diff #188885)

Alright, I don't completely understand why Twine is the way that it is, but here:

Twine ManglePublicSymbol(StringRef Name)

When we construct Twine(const char*, StringRef), the newly-minted Twine contains a _pointer to_ the passed-in StringRef. It's invalid immediately after ManglePublicSymbol returns. After a few layers of stack pop off, we end up with random garbage and undefined behavior.

A quick and effective fix is to switch Name to be of type const Twine&.

Twine ManglePublicSymbol(const Twine& Name)

Name ends up being a twine rvalue with a LHSType of cString, and all is right in the world.

theraven updated this revision to Diff 193002.Mar 31 2019, 2:22 AM
theraven marked 2 inline comments as done.
  • Fix ownership with Twine.
This revision was not accepted when it landed; it landed in state Needs Review.Mar 31 2019, 4:21 AM
This revision was automatically updated to reflect the committed changes.