NSS_3_36_BETA3 doesn't not compile under VS2015: security/nss/lib/freebl/verified/Hacl_Chacha20_Vec128.c(229): error C2719: 'v3': formal parameter with requested alignment of 16 won't be aligned

RESOLVED FIXED in 3.36

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jorgk, Unassigned)

Tracking

other
3.36
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Thunderbird-testfailure: B Win32 VS2015])

Attachments

(2 attachments, 1 obsolete attachment)

We are seeing compile errors on Thunderbird 32bit builds on builbot which are still using VS2015:
security/nss/lib/freebl/verified/Hacl_Chacha20_Vec128.c(229): error C2719: 'v3': formal parameter with requested alignment of 16 won't be aligned

This seems to have regressed by:
3aea87ef84e9 Kai Engert — Bug 1432177, uplift NSS_3_36_BETA3, r=me

Also reported as bug 1432177 comment #9.
Flags: needinfo?(kaie)
Flags: needinfo?(franziskuskiefer)
Whiteboard: [Thunderbird-testfailure: B Win32 VS2015]
Franziskus, in bug 1432177 comment 11 you said:

> Hm, what exactly is the setup you use to build?

Jorg saiv: VS2015

> The issue you are seeing is that __m128i have to have an alignment of 16.
> But I wouldn't expect that to be an issue here (and didn't see it anywhere yet).

The issue is that it doesn't build, which is probably a blocker for Thunderbird.

Could this be easy to fix?

I see that parameter type "vec" is a typedef and equivalent to the __m128i type the Franziskus mentioned.

There are 4 parameters of type vec. Why does the compiler complain about the last parameter, only?

Could the non-alignment be caused by the first parameter? Could this get fixed by reordering the parameters?
Flags: needinfo?(kaie)
Posted patch 1442554-v1.patch (obsolete) — Splinter Review
Jorg, could you please test if this patch fixes the compiler error?
Attachment #8955444 - Flags: feedback?(jorgk)
I don't think that your patch helps Kai.
First, I guess there should be VS2015 in CI if that's still something that's supported.
Second, we'd have to align all `vec` variables, which are __m128i here that require alignment of 16. That's not a big deal but we should do it properly and I currently don't have a way to test it.
Flags: needinfo?(franziskuskiefer)
This VS2015 documentation page:
  https://msdn.microsoft.com/en-us/library/26232t5c.aspx

says:
  "Variables of type _m128i are automatically aligned on 16-byte boundaries."

That seems like a contradiction. The doc says it will automatically align them, but then fails with an error because aligning is not possible? Compiler bug?

However, this page:
  https://msdn.microsoft.com/en-us/library/373ak2y1.aspx

says:
  "The align __declspec modifier is not permitted on function parameters."

Maybe you should rather provide the parameters by reference, not by value.
I'll suggest another patch.
Attachment #8955444 - Attachment is obsolete: true
Attachment #8955444 - Flags: feedback?(jorgk)
Jorg, could you test this patch?
Attachment #8955447 - Flags: review?(franziskuskiefer)
Attachment #8955447 - Flags: feedback?(jorgk)
Thanks for the quick action here. It's not a blocker since currently we have Builtbot and TaskCluster builds and the failure is only on Builtbot for Windows 32 bit.

My development machines are all set up for 64 bit compile, so here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cafb737f4667291314b3bc29ee25f500b0028826
Duplicate of this bug: 1442605
(In reply to Jorg K (GMT+1) from comment #6)
> Thanks for the quick action here. It's not a blocker since currently we have
> Builtbot and TaskCluster builds and the failure is only on Builtbot for
> Windows 32 bit.

We want to release this NSS version on Monday/Tuesday, so from my side I'd like to get it fixed ASAP and the fix included in the Monday/Tuesday NSS release.

> My development machines are all set up for 64 bit compile, so here's a try
> run:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=cafb737f4667291314b3bc29ee25f500b0028826

Thanks a lot. It seems that build passed!
Comment on attachment 8955447 [details] [diff] [review]
1442554-v2.patch

Yes, thanks, that compiled.
Attachment #8955447 - Flags: feedback?(jorgk) → feedback+
Comment on attachment 8955447 [details] [diff] [review]
1442554-v2.patch

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

We can't touch this file manually. I've uploaded a version on phab that fixes this.
We can fix it properly on trunk later. But that should work for the 3.36 release.
Attachment #8955447 - Flags: review?(franziskuskiefer)
Comment on attachment 8955533 [details]
Bug 1442554 - inline store_4_vec to fix win32 vs2015 builds, r=kaie,ttaubert

Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D671
Attachment #8955533 - Flags: review+
Assignee: nobody → nobody
Component: Security: PSM → Libraries
Product: Core → NSS
Version: unspecified → other
https://hg.mozilla.org/projects/nss/rev/87d1da047952191d05df7279d07e2508b4d26fe9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.36
You need to log in before you can comment on or make changes to this bug.