-Wpragma-pack warning spam from including pkcs11.h

RESOLVED FIXED in 3.39

Status

defect
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: froydnj, Assigned: emk)

Tracking

(Blocks 2 bugs)

trunk
3.39
All
Windows
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

Last year
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
Assignee

Comment 2

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

Maybe the corresponding clang pragma should be added.
Reporter

Comment 3

11 months ago
(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.
Assignee

Comment 5

11 months ago
Yeah, MSVC doesn't pop the warning pragma, either.
Assignee

Comment 6

11 months ago
My bug 1090497 patch depends on this.
Attachment #8996739 - Flags: review?(core-build-config-reviews)
Assignee

Updated

11 months ago
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+

Comment 8

11 months ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69259eedc1e6
Temporarily suppress -Wpragma-pack warning spam globally. r=dmajor
Assignee

Updated

11 months ago
Keywords: leave-open
Assignee

Comment 10

11 months ago
Attachment #8998342 - Flags: review?(franziskuskiefer)
Assignee

Updated

11 months ago
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.
Assignee

Comment 12

11 months ago
(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.
Assignee

Comment 14

11 months ago
(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)
Assignee

Comment 17

11 months ago
(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)
Assignee

Comment 18

11 months ago
Adding `|| defined(_WINDOWS)`
Attachment #8998813 - Flags: review?(franziskuskiefer)
Assignee

Updated

11 months ago
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: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 3.39
Assignee

Comment 21

10 months ago
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+
Assignee

Comment 23

10 months ago
The next NSS merge has already been landed inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e89feb5a66
Oh, nice, that was fast. Thanks!

Comment 25

10 months ago
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.