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

Export ldexp aliases on Windows #56

Merged
merged 1 commit into from
May 11, 2014
Merged

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented May 8, 2014

The existing strong_reference's in src/s_scalbn.c are not being found since
those files get skipped in favor of the arch-specific assembly versions.
And on Windows, ldexp
needs to be defined with a DLLEXPORT to be resolved.

Fixes JuliaLang/julia#6777

This could be made platform-dependent, using a __strong_reference on Unix and this DLLEXPORT version only on Windows, if there are performance implications of these being functions.

@tkelman
Copy link
Contributor Author

tkelman commented May 8, 2014

Hm, crap. Maybe don't merge this, this might be better to fix at the assembly level. I didn't notice https://github.com/JuliaLang/openlibm/blob/master/amd64/s_scalbn.S#L46-L47, those probably just need exports on Windows.

@vtjnash
Copy link
Contributor

vtjnash commented May 8, 2014

those don't appear to work on windows. in julia, we switched to simply using the name of the actual function rather than the alias

@tkelman
Copy link
Contributor Author

tkelman commented May 8, 2014

Yes, for the ccall's that use these. This cropped back up again on Monday because LLVM is optimizing some powf into ldexpf, according to Jeff in 6777. I think I'm getting something workable, in the C for Win32 and in the assembly for Win64.

@Keno
Copy link
Contributor

Keno commented May 8, 2014

Maybe we need to define the appropriate _imp__ symbols directly?

@Keno
Copy link
Contributor

Keno commented May 8, 2014

Actually, never mind. The way it works is by putting things in the .drectve section (see e.g. http://gcc.gnu.org/ml/gcc-patches/1999-06n/msg00448.html)

@tkelman
Copy link
Contributor Author

tkelman commented May 8, 2014

That's exactly what I was trying to do, going off the example set by bsd_asm.h. Can't seem to get it to work in assembly for win64, but I suspect it's the .set CNAME(ldexp),CNAME(scalbn) part that isn't working properly. We could perhaps write the function in assembly using ENTRY(ldexp) since that is working well, but maybe what I already did here in separate C files is close enough after all?

@tkelman
Copy link
Contributor Author

tkelman commented May 9, 2014

I think I'm getting somewhere with the assembly. The .set line appears to be getting ignored because it comes after END(scalbn). If I just skip the END on Windows, along with adding the export directive, it's looking happier.

I like this version much better than what I had yesterday, and it tests cleanly on Win32, Win64, and Linux 64. I can make the C changes Windows-only if there's a performance concern. I can also put END back in after the .set line for Windows if necessary.

@tkelman tkelman changed the title split references for ldexp out into independent files Export ldexp aliases on Windows May 9, 2014
@JeffBezanson
Copy link
Contributor

Yes, let's make the C changes windows-only.

@Keno
Copy link
Contributor

Keno commented May 9, 2014

Have you tried DLLEXPORT __strong_reference

@tkelman
Copy link
Contributor Author

tkelman commented May 9, 2014

I'm pretty sure I did but I'll try it one more time just to be sure.

@Keno
Copy link
Contributor

Keno commented May 9, 2014

Actually, it seems that in our cdefs-compat.h we have

#ifdef __GNUC__
#ifndef __strong_reference
#define __strong_reference(sym,aliassym)    
    //extern __typeof (sym) aliassym __attribute__ ((__alias__ (#sym)));
#endif /* __strong_reference */

which might be the reason it doesn't work?

@tkelman
Copy link
Contributor Author

tkelman commented May 9, 2014

Hmm, blame sez 8d25b39 "What is __strong_reference?" indeed... @vtjnash do you remember what the problem was on Mac from that?

@vtjnash
Copy link
Contributor

vtjnash commented May 9, 2014

IIRC, mach-o doesn't have a notion of strong references so it can't compile with them

@tkelman
Copy link
Contributor Author

tkelman commented May 9, 2014

Thanks. So does the latest commit I just pushed not work on Mac then?

@vtjnash
Copy link
Contributor

vtjnash commented May 9, 2014

correct. it fails with:

src/s_nextafterl.c:80:1: error: only weak aliases are supported on darwin
__strong_reference(nextafterl, nexttowardl);

@tkelman
Copy link
Contributor Author

tkelman commented May 9, 2014

OK thanks, bummer. Blank definition #ifdef __APPLE__ then?

@Keno
Copy link
Contributor

Keno commented May 9, 2014

Or maybe weak references instead?

@tkelman
Copy link
Contributor Author

tkelman commented May 9, 2014

DLLEXPORT __weak_reference does not work at the moment since that's being done with inline assembly:

In file included from src/s_scalbn.c:20:0:
D:/code/msys64/home/Tony/julia/deps/openlibm/include/cdefs-compat.h:65:2: error: expected identifier or '(' before '__asm__'
  __asm__(".stabs \"_" #alias "\",11,0,0,0"); \
  ^
src/s_scalbn.c:65:11: note: in expansion of macro '__weak_reference'
 DLLEXPORT __weak_reference(scalbn, ldexp);
           ^
Make.inc:46: recipe for target 'src/s_scalbn.c.o' failed

I don't know how to get the .stabs stuff to export.

@tkelman
Copy link
Contributor Author

tkelman commented May 9, 2014

Is this better on Mac now? @StefanKarpinski you have a 32-bit Linux VM now right, does this cause any trouble there?

@Keno
Copy link
Contributor

Keno commented May 9, 2014

What I meant was using weak references on Mac and strong references anywhere else (do we need DLLEXPORT on mac?)

@tkelman
Copy link
Contributor Author

tkelman commented May 9, 2014

No you don't need DLLEXPORT anywhere besides Windows. But evidently these files having to do with ldexp/scalbn are not the only place strong references are used (the error Jameson posted was in s_nextafterl.c). Are you proposing:

#ifdef __APPLE__
#define __strong_reference(sym,aliassym) __weak_reference(sym,aliassym)
#else
#define __strong_reference(sym,aliassym)    \
    extern __typeof (sym) aliassym __attribute__ ((__alias__ (#sym)));
#endif /* __APPLE__ */

And in the locations here where I needed DLLEXPORT, doing

#ifdef _WIN32
DLLEXPORT __strong_reference(scalbn, ldexp);
#else
__strong_reference(scalbn, ldexp);
#endif

?

@Keno
Copy link
Contributor

Keno commented May 9, 2014

Or just

#ifdef __APPLE__
#define __strong_reference(sym,aliassym) __weak_reference(sym,aliassym)
#else
#define __strong_reference(sym,aliassym)    \
    DLLEXPORT extern __typeof (sym) aliassym __attribute__ ((__alias__ (#sym)));
#endif /* __APPLE__ */

@tkelman
Copy link
Contributor Author

tkelman commented May 9, 2014

I'll give that a try, though it'll introduce more DLLEXPORT's that we may or may not need. That might be harmless, let's see.

skip END and add .drectve export in assembly versions

uncomment __strong_reference definition from cdefs-compat.h

use weak references in place of strong references on Mac

add DLLEXPORT to all strong references

Fixes Julia issue #6777
@tkelman
Copy link
Contributor Author

tkelman commented May 10, 2014

Alright this passes Julia tests cleanly on Win32, Win64, and Linux 64. Is it okay on Mac and Linux 32?

@nalimilan
Copy link
Contributor

Build passes on Fedora 32 and 64 bits (I can't run the tests unfortunately because they always fail).

@tkelman
Copy link
Contributor Author

tkelman commented May 11, 2014

Thank you for that. Do you mean openlibm tests, Julia tests, or both?

@staticfloat
Copy link
Contributor

Everything works for me on OSX, openlibm tests fail the same way they do on master, and Julia tests all pass fine.

@vtjnash
Copy link
Contributor

vtjnash commented May 11, 2014

As a side note, I wonder why gcc on Apple rejects this construct, while the linker allowed me to create symbol aliases for gfortblas

@nalimilan
Copy link
Contributor

@tkelman The openlibm tests are the ones I can't run. But I've also just run the Julia tests, and they pass.

@tkelman
Copy link
Contributor Author

tkelman commented May 11, 2014

Thanks guys! This mergable then?

JeffBezanson added a commit that referenced this pull request May 11, 2014
Export ldexp aliases on Windows
@JeffBezanson JeffBezanson merged commit 1f3925b into JuliaMath:master May 11, 2014
@tkelman tkelman deleted the ldexp branch May 12, 2014 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LLVM error on Windows, ldexpf could not be resolved
6 participants