rnk added a comment.
Thanks. I think what we really want to do here is reconsider our default for
applying dllimport. Leaving things unannotated is a good safe default for every
environment. In the absence of any flags, clang should assume runtime functions
are statically linked. The linker will synthesize thunks for us. MSVC and GCC
do this and it's fine, if slow, and perhaps confusing to past versions of LLDB.
Then, we should add new, runtime-specific, -cc1 flags (as @pcc and @mgrang said
earlier) to indicate that certain runtimes will be dynamically linked. We
should have separate -cc1 flags for the obj-c runtime and the CRT. The obj-c
code should call some obj-c specific helper that respects the flag instead of
CreateRuntimeFunction. We can do something like that in MicrosoftCXXABI for the
msvc crt functions. As a straw man, I'd propose `-shared-crt` for consistency
with `-shared-libgcc`, `-static-libc++`, and others as the flag spelling. I
could imagine using that on mingw as an optimization for people who don't want
the thunks.
Does that seem reasonable?
================
Comment at: CodeGen/CodeGenModule.cpp:2957-2958
!getCodeGenOpts().LTOVisibilityPublicStd &&
- !getTriple().isWindowsGNUEnvironment()) {
+ !getTriple().isWindowsGNUEnvironment() &&
+ !getTriple().isWindowsMSVCEnvironment()) {
const FunctionDecl *FD = GetRuntimeFunctionDecl(Context, Name);
----------------
compnerd wrote:
> rnk wrote:
> > The condition here of `T.isOSBinFormatCOFF() &&
> > !T.isWindowsGNUEnvironment() && !T.isWindowsMSVCEnvironment()` is
> > essentially equivalent to `T.isWindowsItaniumEnvironment()`. Please
> > simplify the condition to that. The other relevant environments are Cygnus
> > for Cygwin and CoreCLR, which probably want to follow the MSVC/GNU behavior.
> IIRC, one of the issues was that lldb couldn't deal very well with the
> thunks. The other thing is that there is a small penalty for something that
> we know that we are going to redirect through. However, I think that if lldb
> is able to deal with the thunks now, I would be okay with the penalty to make
> the behavior more similar to MSVC. Basically, if lldb works, lets just get
> rid of the special behavior for the itanium environment.
Can you elaborate on what didn't work in LLDB when the thunks were present?
What kind of functionality did you have to exercise to get it to misbehave?
================
Comment at: CodeGen/ms-symbol-linkage.cpp:1-3
+// RUN: %clangxx -target aarch64-windows \
+// RUN: -fcxx-exceptions -c -o - %s \
+// RUN: | llvm-objdump -syms - 2>&1 | FileCheck %s
----------------
Please do not write tests that emit object files in clang. Use %clang_cc1
-emit-llvm like the other tests and check the IR for dllimport or the lack
thereof.
================
Comment at: CodeGenObjC/gnu-init.m:103
+// Make sure we do not have dllimport on the load function
+// CHECK-WIN: declare dso_local void @__objc_load
----------------
compnerd wrote:
> rnk wrote:
> > mgrang wrote:
> > > I had to remove dllimport in this and the next unit test. I am not sure
> > > of the wider implications of this change. Maybe controlling this via a
> > > flag (like -flto-visibility-public-std) is a better/more localized option?
> > These are the ones that I think @compnerd really cares about since the objc
> > runtime is typically dynamically linked and frequently called. We might
> > want to arrange things such that we have a separate codepath for ObjC
> > runtime helpers. I'm surprised we don't already have such a code path.
> I think that @theraven would care more about this code path than I. I think
> that this change may be wrong, because the load here is supposed to be in the
> ObjC runtime, and this becoming local to the module would break the global
> registration.
We just set dso_local whenever something isn't dllimport, even if it's a lie
sometimes. I think everything would work as intended with a linker thunk. It's
"true" as far as LLVM is concerned for the purposes of emitting relocations.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55229/new/
https://reviews.llvm.org/D55229
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
