Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Terra Cannot Import Inlined C Functions #401

ErikMcClure opened this issue Sep 24, 2019 · 2 comments · Fixed by #414

Terra Cannot Import Inlined C Functions #401

ErikMcClure opened this issue Sep 24, 2019 · 2 comments · Fixed by #414


Copy link

@ErikMcClure ErikMcClure commented Sep 24, 2019

As I discovered while trying to get the tests to work again in #400, Terra shouldn't actually be working on windows at all. This is because printf is defined as an inline function and has been since Visual Studio 2015. As a result, printf doesn't exist. You can force it to exist by including legacy_stdio_definitions.lib, but you must include this manually on every single project on windows that uses printf, without disrupting linux compilations, like so:

terralib.saveobj("benchmark_fannkuchredux", { main = main }, (jit.os == "Windows" and {"\\legacy_stdio_definitions.lib"} or nil) )

The larger issue here is that terra cannot actually compile any inline-only function because the function immediately ceases to exist. This also applies to things like compiler intrinsics that don't correspond to an actual function symbol.

The inline-only problem can be fixed by using llvm.used, as was done in #405, but this does not fix the stdio specific symbol resolution problem.

We could also consider trying to just manually include the stdio_legacy_definitions.lib on all windows compilations (plus our own library, if necessary) and therefore ensure that all of the standard library functions have symbols, at the very least. Whatever option is chosen, using the C standard library's printf() function should not result in random, confusing compiler errors that are incredibly hard to track down.

Copy link
Contributor Author

@ErikMcClure ErikMcClure commented Sep 24, 2019

benchmark_fannkuchredux.t and benchmark_nbody.t are being temporarily skipped until we can consistently resolve this across visual studio versions.

Copy link
Contributor Author

@ErikMcClure ErikMcClure commented Oct 1, 2019

The inline function issue has been resolved using llvm.used, but there is still a windows-specific stdio.h problem with duplicate symbols:

msvcrt.lib(default_local_stdio_options.obj) : error LNK2005: __local_stdio_printf_options already defined in terra-19860a.o
stdio.exe : fatal error LNK1169: one or more multiply defined symbols found

This function has the following definition:

// This function must not be inlined into callers to avoid ODR violations.  The
// static local variable has different names in C and in C++ translation units.
_Check_return_ _Ret_notnull_
__declspec(noinline) __inline unsigned __int64* __CRTDECL __local_stdio_printf_options(void)
    static unsigned __int64 _OptionsStorage;
    return &_OptionsStorage;

The linking problem will only occur if printf is called in a function reachable from main, and saveobj is used to save an executable. Clang itself does not run into this problem, but seems to declare this symbol as ExternallyAvailable:

$__local_stdio_printf_options = comdat any

@"??_C@_06FJFFADID@C?3?5?$CFi?6?$AA@" = linkonce_odr dso_local unnamed_addr constant [7 x i8] c"C: %i\0A\00", comdat, align 1
@__local_stdio_printf_options._OptionsStorage = internal global i64 0, align

The LLVM that Terra gets back from clang, however, is an actual function definition:

; Function Attrs: noinline norecurse nounwind
define linkonce_odr dso_local i64* @__local_stdio_printf_options() local_unnamed_addr #4 {
  ret i64* @__local_stdio_printf_options._OptionsStorage

It is possible clang has a substitute stdio.h file it uses to compensate for Windows being insane. It's not obvious how or if it's even possible to fix this problem, or if it will cause issues with other windows header files in the future. Manually setting this function to ExternallyAvailable actually solves the problem, which confirms that the primary issue here is that the function body shouldn't exist, yet it does anyway. How does clang know to avoid emitting the function body? Why does terra's invocation of LLVM insist on giving it one?

if(fn->getName().equals("__local_stdio_printf_options") || fn->getName().equals("__local_stdio_scanf_options"))

ErikMcClure pushed a commit to ErikMcClure/terra that referenced this issue Oct 21, 2019
elliottslaughter added a commit that referenced this issue Oct 22, 2019
* Fix #401 stdio.h include problem

* Favor weakODR linkage over llvm.used in attempt to fix llvm 3.5

* Fix formatting

* Removed unused code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

1 participant