[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

I've been thinking more about your example with static locals in lambda and how this works with regular dllexport.

It got me thinking more about the problem of exposing inline functions from a library. For example:

`lib.h`:

  #ifndef LIB_H
  #define LIB_H
  
  int foo();
  
  struct __declspec(dllimport) S {
    int bar() { return foo(); }
  };
  
  #endif

`lib.cc`:

  #include "lib.h"
  
  int foo() { return 123; }

`main.cc`:

  #include "lib.h"
  
  int main() {
    S s;
    return s.bar();
  }

Here, Clang will not emit the body of `S::bar()`, because it references the non-dllimport function `foo()`, and trying to referencing `foo()` outside the library would result in a link error. This is what the `DLLImportFunctionVisitor` is used for. For the same reason, MSVC will also not inline dllimport functions.

Now, with `/Zc:dllexportInlines-`, we no longer mark `S::bar()` dllimport, and so we do emit it, causing that link error. The same problem happens with `-fvisibility-inlines-hidden`: substitute the `declspec` above for `__attribute__((visibility("default")))` above and try it:

  $ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o lib.so lib.cc
  $ g++ main.cc lib.so
  /tmp/cc557J3i.o: In function `S::bar()':
  main.cc:(.text._ZN1S3barEv[_ZN1S3barEv]+0xd): undefined reference to `foo()'

So this is a known issue with `-fvisibility-inlines-hidden`. This doesn't come up often in practice, but when it does the developer needs to deal with it.

However, the static local problem is much scarier, because that leads to run-time bugs:

`lib.h`:

  #ifndef LIB_H
  #define LIB_H
  
  inline int foo() { static int x = 0; return x++; }
  
  struct __attribute__((visibility("default"))) S {
    int bar() { return foo(); }
    int baz();
  };
  
  #endif

`lib.cc`:

  #include "lib.h"
  
  int S::baz() { return foo(); }

`main.cc`:

  #include <stdio.h>
  #include "lib.h"
  
  int main() {
    S s;
    printf("s.bar(): %d\n", s.bar());
    printf("s.baz(): %d\n", s.baz());
    return 0;
  }

If we build the program normally, we get the expected output:

  $ g++ lib.cc main.cc && ./a.out
  s.bar(): 0
  s.baz(): 1

but building as a shared library shows the problem:

  $ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o lib.so lib.cc
  $ g++ main.cc lib.so
  $ LD_LIBRARY_PATH=$PWD:$LD_LIBRARY_PATH ./a.out
  s.bar(): 0
  s.baz(): 0

Oops.

This does show that it's a pre-existing problem with the model of not exporting inline functions though. I don't think we need to solve this specifically for Windows, I think we should match what `-fvisibility-inlines-hidden` is doing.

And what about the lambdas?

`lib.h`:

  #ifndef LIB_H
  #define LIB_H
  
  struct __attribute__((visibility("default"))) S {
    int bar() { return ([]() { static int x; return x++; })(); }
    int baz();
  };
  
  #endif

`lib.cc`:

  #include "lib.h"
  
  int S::baz() { return bar(); }

  $ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o lib.so lib.cc && g++ main.cc lib.so && LD_LIBRARY_PATH=$PWD:$LD_LIBRARY_PATH ./a.out
  s.bar(): 0
  s.baz(): 1

Interesting, the lambda function and its static local are not hidden.

Either that's because they're applying the "static locals of hidden functions in visible decl contexts are visible" thing. Or alternatively, they never applied the hidden visibility to the lambda in the first place.

I'm not entirely sure what this means for the dllexport case though. Maybe the loop in the current patch is fine.


https://reviews.llvm.org/D51340