Closed Bug 1472254 Opened 2 years ago Closed 2 years ago

-Wpragma-pack warning spam from including pkcs11.h

Categories

(NSS :: Build, defect)

All
Windows
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: froydnj, Assigned: emk)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

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?
(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
The warning is disabled in genuine MSVC because of a pragma:
>  #pragma warning(disable : 4103)

Maybe the corresponding clang pragma should be added.
(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.
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.
Yeah, MSVC doesn't pop the warning pragma, either.
My bug 1090497 patch depends on this.
Attachment #8996739 - Flags: review?(core-build-config-reviews)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
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.
Attachment #8996739 - Flags: review?(core-build-config-reviews) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69259eedc1e6
Temporarily suppress -Wpragma-pack warning spam globally. r=dmajor
Keywords: leave-open
Attachment #8998342 - Flags: review?(franziskuskiefer)
Keywords: leave-open
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.
(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.
(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.
(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?
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 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.
Attachment #8998342 - Flags: review?(franziskuskiefer)
(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
Flags: needinfo?(franziskuskiefer)
Adding `|| defined(_WINDOWS)`
Attachment #8998813 - Flags: review?(franziskuskiefer)
Attachment #8998342 - Attachment is obsolete: true
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!
Attachment #8998813 - Flags: review?(franziskuskiefer) → review+
Flags: needinfo?(franziskuskiefer)
https://hg.mozilla.org/projects/nss/rev/01d970fe90483db538eda1134faad25131f9137d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.39
Attachment #8999165 - Flags: review?(core-build-config-reviews)
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.
Attachment #8999165 - Flags: review?(core-build-config-reviews) → review+
The next NSS merge has already been landed inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e89feb5a66
Oh, nice, that was fast. Thanks!
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d475ec08b13
Backed out changeset 69259eedc1e6 as the final fix has landed. r=dmajor
You need to log in before you can comment on or make changes to this bug.