1442554 - 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

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Jorg K (CEST = GMT+2)]

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.

Comment 1

[Kai Engert (:KaiE:)]

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?

Comment 2

[Kai Engert (:KaiE:)]

Attachment: 1442554-v1.patch

Jorg, could you please test if this patch fixes the compiler error?

Comment 3

[Franziskus Kiefer [:franziskus]]

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.

Comment 4

[Kai Engert (:KaiE:)]

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.

Comment 5

[Kai Engert (:KaiE:)]

Attachment: 1442554-v2.patch

Jorg, could you test this patch?

Comment 6

[Jorg K (CEST = GMT+2)]

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

Comment 8

[Kai Engert (:KaiE:)]

(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 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8955447 [details] [diff] [review]
1442554-v2.patch

Yes, thanks, that compiled.

Comment 10

[Phabricator Automation]

Attachment: Bug 1442554 - inline store_4_vec to fix win32 vs2015 builds, r=kaie,ttaubert

Comment 11

[Franziskus Kiefer [:franziskus]]

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.

Comment 12

[Phabricator Automation]

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

Comment 13

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/87d1da047952191d05df7279d07e2508b4d26fe9