1472254 - -Wpragma-pack warning spam from including pkcs11.h

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Nathan Froyd [:froydnj]]

clang-cl warns repeatedly about the pattern:

/* packing defines */
#include "pkcs11p.h"

...

/* unpack */
#include "pkcs11u.h"

that pkcs11.h decides to use (apologies for the Firefox build, but it's convenient):

15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/media/webrtc/signaling/src/peerconnection/Unified_cpp_src_peerconnection0.cpp:2:
15:22:19     INFO -  In file included from z:/build/build/src/media/webrtc/signaling/src/peerconnection/PacketDumper.cpp:6:
15:22:19     INFO -  In file included from z:/build/build/src/media/webrtc\signaling/src/peerconnection/PeerConnectionImpl.h:46:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include\mozilla/net/DataChannel.h:32:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include\mtransport/transportlayerdtls.h:23:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include\ScopedNSSTypes.h:16:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss\cert.h:21:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss/keyt.h:8:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss/keythi.h:9:
15:22:19     INFO -  z:/build/build/src/obj-firefox/dist/include/nss/pkcs11t.h(75,10):  warning: the current #pragma pack aligment value is modified in the included file [-Wpragma-pack]
15:22:19     INFO -  #include "pkcs11p.h"
15:22:19     INFO -           ^
15:22:19     INFO -  z:/build/build/src/obj-firefox/dist/include/nss/pkcs11p.h(20,9):  note: previous '#pragma pack' directive that modifies alignment is here
15:22:19     INFO -  #pragma pack(push, cryptoki, 1)
15:22:19     INFO -          ^
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/media/webrtc/signaling/src/peerconnection/Unified_cpp_src_peerconnection0.cpp:2:
15:22:19     INFO -  In file included from z:/build/build/src/media/webrtc/signaling/src/peerconnection/PacketDumper.cpp:6:
15:22:19     INFO -  In file included from z:/build/build/src/media/webrtc\signaling/src/peerconnection/PeerConnectionImpl.h:46:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include\mozilla/net/DataChannel.h:32:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include\mtransport/transportlayerdtls.h:23:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include\ScopedNSSTypes.h:16:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss\cert.h:21:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss/keyt.h:8:
15:22:19     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss/keythi.h:9:
15:22:19     INFO -  z:/build/build/src/obj-firefox/dist/include/nss/pkcs11t.h(1800,10):  warning: the current #pragma pack aligment value is modified in the included file [-Wpragma-pack]
15:22:19     INFO -  #include "pkcs11u.h"
15:22:19     INFO -           ^
15:22:19     INFO -  note: previous '#pragma pack' directive that modifies alignment is here

This warning spam is really unhelpful, especially because it comes about whenever you include pkcs11.h and I can't see a useful reason for pkcs11.h to delegate things to separate headers in this case.  Could we please merge pkcs11{p,u}.h into pkcs11.h and thereby make this warning go away?

Comment 1

[(Away)]

(In reply to Nathan Froyd [:froydnj] from comment #0)
> This warning spam is really unhelpful, especially because it comes about
> whenever you include pkcs11.h and I can't see a useful reason for pkcs11.h
> to delegate things to separate headers in this case.  Could we please merge
> pkcs11{p,u}.h into pkcs11.h and thereby make this warning go away?

I didn't write this code but as a reader I get the impression that the {p,u} headers were designed to be reusable. I see at least three files that include them: https://searchfox.org/mozilla-central/search?q=pkcs11p.h

Comment 2

[Masatoshi Kimura [:emk]]

The warning is disabled in genuine MSVC because of a pragma:
>  #pragma warning(disable : 4103)

Maybe the corresponding clang pragma should be added.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Masatoshi Kimura [:emk] from comment #2)
> The warning is disabled in genuine MSVC because of a pragma:
> >  #pragma warning(disable : 4103)
> 
> Maybe the corresponding clang pragma should be added.

This is a reasonable idea; testing a patch.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I would hope that we don't disable that warning for every file includes them, but I cannot figure out a reasonable approach otherwise... Since the warning is issues on the #include rather than from included file, we cannot pop the diagnostic pragma in pkcs11u.h which means we have to accept it being spread into the rest of compilation unit.

Comment 5

[Masatoshi Kimura [:emk]]

Yeah, MSVC doesn't pop the warning pragma, either.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Temporarily suppress -Wpragma-pack warning spam globally

My bug 1090497 patch depends on this.

Comment 7

[(Away)]

Comment on attachment 8996739 [details] [diff] [review]
Temporarily suppress -Wpragma-pack warning spam globally

I'd really prefer to see this fixed with `#pragma clang diagnostic` in the NSS files. But I know that changing NSS files is time-consuming due to the extra upstream repo, and I understand the desire to land bug 1090497 before anything else regresses. Also, oh boy do I want to get rid of this Wpragma-pack spam in my own local builds.

I won't demand it, but I'd definitely appreciate it if you'd also sign up to do the NSS change as a followup.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69259eedc1e6
Temporarily suppress -Wpragma-pack warning spam globally. r=dmajor

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/69259eedc1e6

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Suppress -Wpragma-pack warning spam

Comment 11

[(Away)]

This is going to disable -Wpragma-pack for any compilation unit that includes pkcs11p.h, right?

Can we surround these with a push/pop? My understanding of this warning is that it will need to be ignored from outside of the header, i.e. in any file that includes pkcs11p.h.

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to David Major [:dmajor] from comment #11)
> This is going to disable -Wpragma-pack for any compilation unit that
> includes pkcs11p.h, right?
> 
> Can we surround these with a push/pop? My understanding of this warning is
> that it will need to be ignored from outside of the header, i.e. in any file
> that includes pkcs11p.h.

No, we can't. See comment #4. I also double-checked that.

Although we can add push/pop outside of the headers, it will defeat the sole purpose of the headers.
I think these headers were introduced to avoid writing

  #if defined(_WIN32)
  #pragma pack(push, cryptoki, 1)
  #endif

and

  #if defined(_WIN32)
  #pragma pack(pop, cryptoki)
  #endif

every time. If we will have to write

  #ifdef __clang__
  #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wpragma-pack"
  #endif
  #ifdef _MSC_VER
  #pragma warning(push)
  #pragma warning(disable : 4103)
  #endif
  #include "pkcs11p.h"
  #ifdef __clang__
  #pragma clang diagnostic pop
  #endif
  #ifdef _MSC_VER
  #pragma warning(pop)
  #endif

and

  #ifdef __clang__
  #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wpragma-pack"
  #endif
  #ifdef _MSC_VER
  #pragma warning(push)
  #pragma warning(disable : 4103)
  #endif
  #include "pkcs11u.h"
  #ifdef __clang__
  #pragma clang diagnostic pop
  #endif
  #ifdef _MSC_VER
  #pragma warning(pop)
  #endif

every time, there is no point in having these headers at all.

Comment 13

[(Away)]

(In reply to Masatoshi Kimura [:emk] from comment #12)
> If we will have to write
...
> every time, there is no point in having these headers at all.

We could still keep the _MSC_VER/4103 code inside of the headers. And the core logic of the packing size (i.e. the number `1`) would still only be in one place. Personally I like having a single point of truth for the real shipping code and don't mind duplicating "maintenance" code like warning management.

IMO the current patch makes for too much "surprise" -- some code in a translation unit may or may not get -Wno-pragma-pack, depending on whether (or when) it included the pkcs headers. If we can't do the proper thing with push/pop, we might as well disable -Wpragma-pack globally at the build system level.

Anyway this should probably be decided by an NSS peer.

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to David Major [:dmajor] from comment #13)
> We could still keep the _MSC_VER/4103 code inside of the headers.

Why? If push/pop is added inside the headers, MSVC will spew warnings just like clang-cl, and build will fail due to warnings-as-errors (I confirmed). If push/pop is not added, why MSVC is allowed to change warnings without restoring them while clang-cl is not?

Comment 15

[(Away)]

Oh, sorry, I wasn't reading the MSVC code carefully. I thought it was already doing a push/pop, or a pragma suppress.

So basically, MSVC is already doing the "surprising" thing. I'm pretty sad to learn that. :-/

Comment 16

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8998342 [details] [diff] [review]
Suppress -Wpragma-pack warning spam

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

I'm fine with the __clang__ change but the Windows define is broken in gyp builds.

::: lib/util/pkcs11u.h
@@ +10,5 @@
>  /*
>   * reset any packing set by pkcs11p.h
>   */
>  
>  #if defined(_WIN32)

There are still a couple of these _WIN32 in NSS. But that's not defined in gyp builds, and shouldn't really be defined in Firefox NSS builds (but is). So it would be good to have a _WINDOWS or WIN95 here as well.

Comment 17

[Masatoshi Kimura [:emk]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #16)
> There are still a couple of these _WIN32 in NSS. But that's not defined in
> gyp builds, and shouldn't really be defined in Firefox NSS builds (but is).
> So it would be good to have a _WINDOWS or WIN95 here as well.

Really? According to MSDN, _WIN32 is always predefined by the compiler.[1]
Rather, _WINDOWS or WIN95 is not be guaranteed to be defined.

[1] https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Suppress -Wpragma-pack warning spam

Adding `|| defined(_WINDOWS)`

Comment 19

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8998813 [details] [diff] [review]
Suppress -Wpragma-pack warning spam

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

> Really? According to MSDN, _WIN32 is always predefined by the compiler.[1] Rather, _WINDOWS or WIN95 is not be guaranteed to be defined.

The gyp build defines _WINDOWS and WIN95. _WIN32 isn't defined there. I'm not sure why. But I just ran into that issue somewhere else. So I think adding the || here is the safe option.
Thanks!

Comment 20

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/01d970fe90483db538eda1134faad25131f9137d

Comment 21

[Masatoshi Kimura [:emk]]

Attachment: Backed out changeset 69259eedc1e6 as the final fix has landed

Comment 22

[(Away)]

Comment on attachment 8999165 [details] [diff] [review]
Backed out changeset 69259eedc1e6 as the final fix has landed

We should wait until the next NSS merge before reverting this.

Comment 23

[Masatoshi Kimura [:emk]]

The next NSS merge has already been landed inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e89feb5a66

Comment 24

[(Away)]

Oh, nice, that was fast. Thanks!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d475ec08b13
Backed out changeset 69259eedc1e6 as the final fix has landed. r=dmajor

Comment 26

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/1d475ec08b13