[COFF] Statically link certain runtime library functions

[rnk]

Summary

Statically link certain runtime library functions for MSVC/GNU Windows environments. This is consistent with MSVC behavior.

[mgrang]

I wonder why this flag is called -flto-visibility-public-std. It has nothing to do with -flto. While we are at it, does it make sense to rename this flag?

[mstorsjo]

FWIW, I don't see how this patch is related to ARM64 (the subject/tag of the patch). Using that for the test probably is fine as it can use any architecture, although an x86 one probably would increase the chances of it getting run by everyone.

I don't have much input on the rest of it, although the name of this flag has puzzled me also when I've run into it.

[pcc]

The reason why it has that name is that it was originally added to support the logic here: http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CGVTables.cpp#991

It looks like it was later repurposed as a "was /MT passed to the driver" flag when the logic was added to mark runtime functions as dllimport depending on the value of the flag.

It certainly seems to make sense to rename it since the name makes no sense as a driver flag and is now being used for other purposes.

[mgrang]

In D55229#1317232, @pcc wrote:

The reason why it has that name is that it was originally added to support the logic here: http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CGVTables.cpp#991

It looks like it was later repurposed as a "was /MT passed to the driver" flag when the logic was added to mark runtime functions as dllimport depending on the value of the flag.

It certainly seems to make sense to rename it since the name makes no sense as a driver flag and is now being used for other purposes.

How about calling it -fmsvc-static-link?

[rnk]

I think @compnerd arranged things this way. I don't know why -flto-visibility-public-std is involved, I have no idea what that does. I think we should do what MSVC does here, which is to leave _CxxThrowException unannotated and let the linker synthesize import thunks.

Here is the code in question that applies dllimport: https://github.com/llvm-git-prototype/llvm/blob/master/clang/lib/CodeGen/CodeGenModule.cpp#L2955

My suggestion would be to make it check isWindowsItaniumEnvironment so we can avoid this behavior which isn't desired under either mingw or MSVC.

[mgrang]

In D55229#1317333, @rnk wrote:

I think @compnerd arranged things this way. I don't know why -flto-visibility-public-std is involved, I have no idea what that does. I think we should do what MSVC does here, which is to leave _CxxThrowException unannotated and let the linker synthesize import thunks.

Here is the code in question that applies dllimport: https://github.com/llvm-git-prototype/llvm/blob/master/clang/lib/CodeGen/CodeGenModule.cpp#L2955

My suggestion would be to make it check isWindowsItaniumEnvironment so we can avoid this behavior which isn't desired under either mingw or MSVC.

Even with the isWindowsItaniumEnvironment check I see it generates impCxxThrowException and break the unit test CodeGenCXX/runtime-dllstorage.cpp. However, with the isOSWindows check it generates __CxxThrowException but ends up breaking a few more unit tests.

[mgrang]

CodeGenObjC/gnu-init.m

103

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?

[mgrang]

Ping for reviews please.

[rnk]

Sorry, I was out sick for a week. We should ask @smeenai about this change, since he has been doing more work in this area recently.

CodeGen/CodeGenModule.cpp

2957–2958

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.

CodeGenObjC/gnu-init.m

103

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.

[compnerd]

CodeGen/CodeGenModule.cpp

2957–2958

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.

CodeGenObjC/gnu-init.m

103

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.

[rnk]

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?

CodeGen/CodeGenModule.cpp

2957–2958

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?

CodeGen/ms-symbol-linkage.cpp

1–3

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.

CodeGenObjC/gnu-init.m

103

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.

[compnerd]

CodeGen/CodeGenModule.cpp

2957–2958

Sure, it was single stepping through an indirected function call. It would put you into the thunk rather than the destination.

CodeGenObjC/gnu-init.m

103

Ah, okay, then, this might be okay. However, the use of dllimport here would force that the runtime to be called. Then again it is possible to statically link ...

[theraven]

If we're going to change the default, please at least add a flag to allow callers to opt into dlimport. We can then make that dependent on -static-objc or similar.

CodeGenObjC/gnu-init.m

103

__objc_load is a function defined in objc.dll. It absolutely does want to be dlimport, or the linker won't be able to find it.

CodeGenObjCXX/msabi-stret.mm

16

Similarly, the objc_msgSend family are defined in objc.dll and so need to be dlimported.

[rnk]

CodeGenObjC/gnu-init.m

103

I don't agree, it doesn't have to be marked dllimport. MSVC, ld.bfd, and lld will all synthesize a linker thunk so that the call works. Marking it dllimport is a performance optimization.

[abdulras]

@rnk, I agree that it is a performance optimization, and it actually is in the hot path. I can understand the motivation for the jump. I think that the trampoline might also be a problem for lldb as it (or used to?) statically analyzes to see if it is jumping to objc_msgSend (yes, it is ugly).

[rnk]

This patch fixes a lot of LNK4286 warnings when running check-asan on Windows, so I'd like to get it committed upstream. Are there any remaining objections? Is it OK if I commandeer the revision and make some minor aesthetic adjustments and land it?

[rnk]

In D55229#1455472, @rnk wrote:

This patch fixes a lot of LNK4286 warnings when running check-asan on Windows, so I'd like to get it committed upstream. Are there any remaining objections? Is it OK if I commandeer the revision and make some minor aesthetic adjustments and land it?

I'll go ahead and do this and try to get it rebased.

[rnk]

• rebase

[jfb]

Looks like this might have broken bots:

/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm/tools/clang/test/CodeGenCXX/runtime-dllstorage.cpp:121:26: error: CHECK-IA-DAG: expected string not found in input
// CHECK-DYNAMIC-IA-DAG: declare dllimport i32 @__cxa_thread_atexit(void (i8*)*, i8*, i8*)
                         ^
<stdin>:1:1: note: scanning from here
; ModuleID = '/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm/tools/clang/test/CodeGenCXX/runtime-dllstorage.cpp'
^
<stdin>:42:1: note: possible intended match here
declare dso_local i32 @__cxa_thread_atexit(void (i8*)*, i8*, i8*) #2
^

http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/60461/

[rnk]

In D55229#1479712, @jfb wrote:

Looks like this might have broken bots:

I replied on Thursday, but it looks like it didn't make it to Phab:

I thought I reverted it, did I not push that?

Looking now, it seems I did push the revert in rL359251. Sorry about that.

[rnk]

• update test to check IR

CodeGen/CodeGenModule.cpp

llvm::Constant *C =
                               ExtraAttrs);
 
   if (auto *F = dyn_cast<llvm::Function>(C)) {
     if (F->empty()) {
       F->setCallingConv(getRuntimeCC());
 
       if (!Local && getTriple().isOSBinFormatCOFF() &&
           !getCodeGenOpts().LTOVisibilityPublicStd &&
-          !getTriple().isWindowsGNUEnvironment()) {
+          !getTriple().isWindowsGNUEnvironment() &&
+          !getTriple().isWindowsMSVCEnvironment()) {
         const FunctionDecl *FD = GetRuntimeFunctionDecl(Context, Name);
         if (!FD || FD->hasAttr<DLLImportAttr>()) {
           F->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
           F->setLinkage(llvm::GlobalValue::ExternalLinkage);
         }
       }
       setDSOLocal(F);
     }

CodeGen/ms-symbol-linkage.cpp

+// RUN: %clangxx -target aarch64-windows \
+// RUN: -fcxx-exceptions -c -o - %s \
+// RUN: | llvm-objdump -syms - 2>&1 | FileCheck %s
+
+void foo1() { throw 1; }
+// CHECK-LABEL: foo1
+// CHECK-NOT: __imp__CxxThrowException
+
+void bar();
+void foo2() noexcept(true) { bar(); }
+// CHECK-LABEL: foo2
+// CHECK-NOT: __imp___std_terminate
+
+struct A {};
+struct B { virtual void f(); };
+struct C : A, virtual B {};
+struct T {};
+T *foo3() { return dynamic_cast<T *>((C *)0); }
+// CHECK-LABEL: foo3
+// CHECK-NOT: __imp___RTDynamicCast

CodeGenCXX/runtime-dllstorage.cpp

 
 void l() {
   std::terminate();
 }
 
 // CHECK-MS-DAG: @_Init_thread_epoch = external thread_local global i32
 // CHECK-MS-DAG: declare dso_local i32 @__tlregdtor(void ()*)
 // CHECK-MS-DAG: declare dso_local i32 @atexit(void ()*)
-// CHECK-MS-DYNAMIC-DAG: declare dllimport {{.*}} void @_CxxThrowException
+// CHECK-MS-DYNAMIC-DAG: declare {{.*}} void @_CxxThrowException
 // CHECK-MS-STATIC-DAG: declare {{.*}} void @_CxxThrowException
 // CHECK-MS-DAG: declare dso_local noalias i8* @"??2@YAPAXI@Z"
 // CHECK-MS-DAG: declare dso_local void @_Init_thread_header(i32*)
 // CHECK-MS-DAG: declare dso_local void @_Init_thread_footer(i32*)
 
 // CHECK-IA-DAG: @_ZTH1t = dso_local alias void (), void ()* @__tls_init
 // CHECK-IA-DAG: declare dso_local i32 @__gxx_personality_v0(...)
 // CHECK-IA-DAG: define linkonce_odr hidden void @__clang_call_terminate(i8*)

CodeGenObjC/gnu-init.m

 
 // We shouldn't have emitted any null placeholders on Windows.
 // CHECK-WIN: @llvm.used = appending global [2 x i8*] [i8* bitcast ({ { i8*, i8*, i8*, i32, i32, i32, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i32, i8* }*, i8*, i8*, i32, i32, i32, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i32, i8* }** @._OBJC_INIT_CLASS_X to i8*), i8* bitcast (void ()** @.objc_ctor to i8*)], section "llvm.metadata"
 // CHECK-WIN: @llvm.compiler.used = appending global [1 x i8*] [i8* bitcast (void ()* @.objcv2_load_function to i8*)], section "llvm.metadata"
 
 // Check our load function is in a comdat.
 // CHECK-WIN: define linkonce_odr hidden void @.objcv2_load_function() comdat {
 
-// Make sure we have dllimport on the load function
-// CHECK-WIN: declare dllimport void @__objc_load
+// Make sure we do not have dllimport on the load function
+// CHECK-WIN: declare dso_local void @__objc_load
 

CodeGenObjCXX/msabi-stret.mm

 // RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fobjc-runtime=ios-6.0 -Os -S -emit-llvm -o - %s -mdisable-fp-elim | FileCheck %s
 
 struct S {
   S() = default;
   S(const S &) {}
 };
 
 @interface I
 + (S)m:(S)s;
 @end
 
 S f() {
   return [I m:S()];
 }
 
-// CHECK: declare dllimport void @objc_msgSend_stret(i8*, i8*, ...)
+// CHECK: declare dso_local void @objc_msgSend_stret(i8*, i8*, ...)
 // CHECK-NOT: declare dllimport void @objc_msgSend(i8*, i8*, ...)
-