Closed Bug 1311175 Opened 4 years ago Closed 4 years ago

clang-cl builds fail with unresolved external symbol _s_mp_clamp

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dmajor, Assigned: dmajor)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(ttaubert)
Is there a reason NSS should support clang-cl? It looks to me like a broken attempt to do get a msvc compliant compiler.
Flags: needinfo?(dmajor)
(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.
Flags: needinfo?(dmajor)
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.
Flags: needinfo?(ttaubert)
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.
Attached patch mpi-inlines (obsolete) — Splinter Review
If this is acceptable, would you mind landing it in NSS for me?
Attachment #8804723 - Flags: review?(ttaubert)
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
Attachment #8804723 - Flags: review?(ttaubert) → feedback+
Attached patch mpi-inlinesSplinter Review
Against NSS repo
Attachment #8804723 - Attachment is obsolete: true
Attachment #8805172 - Flags: review?(ttaubert)
Comment on attachment 8805172 [details] [diff] [review]
mpi-inlines

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

Thanks David!
Attachment #8805172 - Flags: review?(ttaubert) → review+
Landed:

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

This should make its way into mozilla-central over the next days.
Assignee: nobody → dmajor
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.28
Duplicate of this bug: 1307411
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.
You need to log in before you can comment on or make changes to this bug.