Thanks for bringing this up. These functions were never meant to be exported publically, we only want to access them for testing. Does that make a difference for the LLVM_ABI tag? Do we have similar situations anywhere else in the codebase?

In order to avoid confusion, we could also make a local LLVM_TEST_ABI redefinition with with a note. Or if this only affects Windows, we could disable the respective unittets there. What do you think?

Thanks for bringing this up. These functions were never meant to be exported publically, we only want to access them for testing. Does that make a difference for the LLVM_ABI tag? Do we have similar situations anywhere else in the codebase?

Because these are not declared in any header there wouldn't really be any expectation that these could be consumed by anything but tests.
Yes i've had to fix missing exports of internal symbols used by tests in other places of the LLVM codebase, sometimes its been internal headers which are easier to automate annotations.

In order to avoid confusion, we could also make a local LLVM_TEST_ABI redefinition with with a note

Maybe i could, but previous discussions i've had with@compnerd he wanted to avoid proliferation of lots of different symbol visibility macros.

if this only affects Windows, we could disable the respective unittets there. What do you think?

Its only affects windows atm, but in the future when default symbol visibility is changed changed to hidden for the LLVM shared library these would need the same annotations as windows

Maybe i could, but previous discussions i've had with@compnerd he wanted to avoid proliferation of lots of different symbol visibility macros.

We will need a macro for each shared library that we support. If we add more macros for each library, it would become a huge number and can be overwhelming for other developers not familiar with this.

@compnerd Couldn't we have one globalLLVM_TEST_ABI macro? I think it's fine to claim that we're either buildingllvm-project (all libraries) for testing or we're not.

I wouldn't want these symbols leaking into the ABI of release builds.

@lhames it seems reasonable at first glance, but quickly breaks down.

Let's say that we are building LLVMSupport as a DLL and LLVM statically. However, becauseLLVM_TEST_ABI is defined once and doesn't consider the target's build type into consideration, we end up with:

#if defined(LLVMSupport_STATIC)#defineLLVM_TEST_ABI/**/#else#if defined(LLVMSupport_EXPORTS)#defineLLVM_TEST_ABI __declspec(dllexport)#else#defineLLVM_TEST_ABI __declspec(dllimport)#endif#endif

Now, when we build LLVM as a static library,LLVMSupport_STATIC is not defined (because it was built dynamically), and because LLVMSupport is not being built here,LLVMSupport_EXPORTS is undefined. So you end up withLLVM_TEST_ABI as__declspec(dllimport), which says that the definitions that LLVM is exporting for use in test are defined elsewhere.

Because the behaviour of the*_ABI macro is dependent on the module being built and how that module is being built, we cannot easily just use a single definition.

Now, maybe, we can get LLVM down to the small enough surface that we can have only 2 libraries (LLVMSupport and LLVM where the second one has ALL architectures enabled and is sufficient to be used for all tools and still is well under 64K entry points including data as part of the ABI). In such a case, yes, it seems reasonable to have two test ABI macros.

My concern is that we might end up having to extract each architecture backend into a separate DLL and then have the optimizations be pulled out, the assembler pulled out, and suddenly you have a set of test ABI macros.

@compnerd Can you dllexport and then dllimport the same symbol within a static link? (Apologies for my lack of windows knowledge here). In that case I'm imagining that we could have something as simple as:

#ifndef NDEBUG#defineLLVM_UNITTEST_EXPORT__declspec(dllexport)#else#defineLLVM_UNITTEST_EXPORT#endif

Then wrap the unit test in#ifndef NDEBUG and unconditionally dllimport the decl?

The declaration and definition must match (that is an error). So the declaration must be__declspec(dllexport) if there is a definition. You cannot have a declaration that is__declspec(dllimport) and then a definition.

That said, if you incorrectly mark the symbol as dllimport and then define it elsewhere within the module, that is generally a linker warning (but it will try to create a thunk to workaround it).

@fsfod Should we land this?

I think so it depends on if@andrurogerz needs it for his work on annotating exports as well.

I think so it depends on if@andrurogerz needs it for his work on annotating exports as well.

I had to make these same patches locally to build the test against an LLVM DLL, so this PR is still relevant. If you're willing/able to merge it that'd be wonderful.

I don't have commit access so could someone merge it for me.