Closed Bug 1487950 Opened 6 years ago Closed 4 years ago

use compiler-provided 128-bit arithmetic for HACL when compiling with clang-cl

Categories

(NSS :: Libraries, enhancement, P3)

x86_64
Windows
enhancement

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1642802

People

(Reporter: froydnj, Unassigned, NeedInfo)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

HACL provides two codepaths for 128-bit arithmetic: one that uses a compiler-provided 128-bit type, and one that operates on pairs of 64-bit integers. The differences between the two also leak into the Poly1305 implementation: the former uses a 64-bit implementation, whereas the latter uses a 32-bit implementation. MSVC doesn't provide a 128-bit integer type, so the current code uses the slower codepaths in both cases. But when we're compiling with clang-cl, we can use its 128-bit integer type directly. This change should result in significantly more efficient code on 64-bit Windows. The attached patch is a proof-of-concept implementation in Firefox's tree; the obvious translation to NSS's repository should work just fine. It may be good to spin up clang-cl builds and tests (bug 1487901) before landing this one, though. This patch does modify kremlib.h, which AFAICT is imported code. I don't know what the right procedure for changing this is; do we modify upstream and then backport?
Attachment #9005783 - Flags: feedback?(franziskuskiefer)
Adding Beurdouche
Flags: needinfo?(benjamin.beurdouche)
Comment on attachment 9005783 [details] use 128-bit integer support when compiling with clang on 64-bit Windows (firefox patch) lgtm in general but we'll have to move the kremlib.h changes upstream and build this on NSS CI.
Attachment #9005783 - Flags: feedback?(franziskuskiefer)
Priority: -- → P3
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #2) > Comment on attachment 9005783 [details] > use 128-bit integer support when compiling with clang on 64-bit Windows > (firefox patch) > > lgtm in general but we'll have to move the kremlib.h changes upstream and > build this on NSS CI. It looks like kremlib.h upstream has already diverged from our packaged copy. Do we want to change upstream and backport the change alone, or do we want to change upstream and then update NSS to conform to upstream's new arrangement?
Flags: needinfo?(franziskuskiefer)
kremlib.h is always a snapshot of the upstream version. So this will go upstream and we update the revision in NSS. Actually, we update the version of HACL* in NSS, which comes with a specific kremlib version. So I expect there to be some issues around this. I'll try to get around to this soonish.
Flags: needinfo?(franziskuskiefer)
QA Contact: jjones

Resolved in bug 1642802.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Target Milestone: --- → 3.54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: