1311175 - clang-cl builds fail with unresolved external symbol _s_mp_clamp

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[(Away)]

This happens only on optimized clang-cl builds.

 0:11.50 ecp_256.obj : error LNK2001: unresolved external symbol _s_mp_clamp
 0:11.50 ecp_384.obj : error LNK2001: unresolved external symbol _s_mp_clamp
 0:11.50 ecp_521.obj : error LNK2001: unresolved external symbol _s_mp_clamp
 0:11.50 mpmontg.obj : error LNK2001: unresolved external symbol _s_mp_clamp
 0:11.50 mplogic.obj : error LNK2001: unresolved external symbol _s_mp_clamp
 0:11.50 mp_gf2m.obj : error LNK2001: unresolved external symbol _s_mp_clamp
 0:11.50 ecl_gf.obj : error LNK2001: unresolved external symbol _s_mp_clamp
 0:11.50 mpmontg.obj : error LNK2019: unresolved external symbol _s_mp_setz referenced in function _s_mp_mul_mont
 0:11.50 d:/build/msys/s/central/obj/asan/security/nss/lib/freebl/freebl3.dll : fatal error LNK1120: 2 unresolved externals

These functions are declared in mpi-priv.h, defined as inline in mpi.c, and called from various .c files in the mpi directory.

It looks like the "inline" keyword is causing those functions not to be visible to callers outside of mpi.c on optimized builds (perhaps the compiler inlines them right then and there, and drops the originals?).

I've worked around this locally by deleting the "inline" but I'm not sure if this is a proper fix. Tim do you have any better ideas?

Comment 1

[Franziskus Kiefer [:franziskus]]

Is there a reason NSS should support clang-cl? It looks to me like a broken attempt to do get a msvc compliant compiler.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #1)
> Is there a reason NSS should support clang-cl? It looks to me like a broken
> attempt to do get a msvc compliant compiler.

Several reasons:

1. We can run sanitizers (ASan, UBSan, etc.) on Windows.
2. We can extend clang-cl with our own static analysis checks.
3. clang-cl checks more stuff by default than MSVC.
4. clang-cl encourages people to fix their broken code.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

According to [1], the current code is somewhat correct for GNU89:

* "inline: the function may be inlined (it's just a hint though). An out-of-line version is always emitted and externally visible. Hence you can only have such an inline defined in one compilation unit, and every other one needs to see it as an out-of-line function (or you'll get duplicate symbols at link time)."

[Problem is of course that they're never inlined outside of mpi.c.]

It's however not for C99:

* "inline: like GNU "extern inline"; no externally visible function is emitted, but one might be called and so must exist"

So there's no out-of-line version of the code in case the compiler decides to not inline when calling these function from outside mpi.c.

To properly inline these with GNU C we'd have to put "extern inline" definitions into the header file, and and out-of-line declarations into mpi.c. For C99 we'd have to put "inline" definitions into the header file and "extern" out-of-line declarations into mpi.c.

These don't mix too well. If we only put "static inline" definitions into mpi-priv.h we'd have C89/99 compatibility but in the worst case we might end up with a copy of these functions in each unit, if the compiler doesn't want to inline.

We could solve that with a few ifdefs. We could alternatively get rid of these inlines and trust the compiler to optimize. I don't think there's any documentation about why these were inlined and whether there's any performance difference with a modern compiler.

Comment 4

[(Away)]

Thanks for the research, Tim.

Given that modern compilers have no hesitation about inlining code with or without the keyword, I'd propose to just drop the inlines.

Comment 5

[(Away)]

Attachment: mpi-inlines

If this is acceptable, would you mind landing it in NSS for me?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8804723 [details] [diff] [review]
mpi-inlines

Review of attachment 8804723 [details] [diff] [review]:
-----------------------------------------------------------------

Please remove the `#define inline` above too. And we would need a patch against the NSS repository [1].

We checked ecperf results on various platforms with various compilers and removing `inline` doesn't seem to have a significant impact.

[1] https://hg.mozilla.org/projects/nss

Comment 7

[(Away)]

Attachment: mpi-inlines

Against NSS repo

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8805172 [details] [diff] [review]
mpi-inlines

Review of attachment 8805172 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks David!

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Landed:

https://hg.mozilla.org/projects/nss/rev/552c9657cb90

This should make its way into mozilla-central over the next days.

Comment 11

[(not currently active) Ted Mielczarek]

Thanks for patching this! I filed a bug but worked around it instead of fixing it. Firefox builds with -Zc:inline on MSVC, which makes this an error.