Closed Bug 1339921 Opened 7 years ago Closed 7 years ago

pkixocsp_VerifyEncodedOCSPResponse.cpp:1002:15: error: non-static data member 'trustDomain' of 'pkixocsp_VerifyEncodedResponse_GetCertTrust' shadows member inherited from type 'pkixocsp_VerifyEncodedResponse' [-Werror,-Wshadow-field]

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file, 1 obsolete file)

bug 1339921 - disable clang's shadowed field warning in a mozilla::pkix gtest class
59 bytes, text/x-review-board-request
Cykesiopka
: review+
dholbert
: review+
Details 
When I build mozilla-central with clang trunk (llvm revision 295219) and ac_add_options --enable-warnings-as-errors, I'm blocked at the following build warning treated as an error:

============
security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp:1002:15: error: non-static data member 'trustDomain' of 'pkixocsp_VerifyEncodedResponse_GetCertTrust' shadows member inherited from type 'pkixocsp_VerifyEncodedResponse' [-Werror,-Wshadow-field]
  TrustDomain trustDomain;
              ^
https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp#1002

security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp:116:23: note: declared here
  OCSPTestTrustDomain trustDomain;
                      ^
https://dxr.mozilla.org/mozilla-central/source/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp#116
============

(DXR links added by me)

I assume this is a new build warning in LLVM -- and it seems like a legitimate thing to be concerned about.

keeler, perhaps you could take a look at this?  (Perhaps we should we rename the member-variable in one of these classes?)

Looks like both of these variables were added in bug 916629 btw (the first in "part 1", the second in "part 4").
Flags: needinfo?(dkeeler)
I think this is actually intentional. The TrustDomain that shadows the inherited member has some logic specific to those testcases, and the shadowing prevents accidentally using the inherited member instead (which would do the wrong thing). I suppose we could rename the member and leave a big warning in the code to take care not to use the wrong one if we ever change something? (It seems like a feature to have the compiler enforce this, although it does make it more confusing and I imagine we can't annotate the code with something that indicates this is intentional, short of a pragma that disables the warning, I guess.)
Flags: needinfo?(dkeeler)
FWIW, this is now the only fatal build warning that I encounter, for a local mozilla-central debug build with clang trunk (llvm revision 297639) and ac_add_options --enable-warnings-as-errors.

It would be great to see this addressed some way or another.
Does that do what I'm intending it to do? (I don't have clang trunk set up, so I figured it would be faster for you to check.)
Assignee: nobody → dkeeler
Priority: -- → P1
Whiteboard: [psm-assigned]
Comment on attachment 8847321 [details]
bug 1339921 - disable clang's shadowed field warning in a mozilla::pkix gtest class

https://reviewboard.mozilla.org/r/120316/#review122246

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp:1006
(Diff revision 1)
>  
> +// trustDomain deliberately shadows the inherited field so that it isn't used
> +// by accident. See bug 1339921.
> +#if defined(__clang__)
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wshadow"

s/shadow/shadow-field/

(Otherwise this doesn't help.)

HOWEVER, this might still be problematic, because older versions of clang won't understand what "-Wshadow-field" means.  If I replace this with "-Wfoo-bar" (to simulate what an older version of clang would experience with this cutting-edge warning), I get this build error:
````
pkixocsp_VerifyEncodedOCSPResponse.cpp:1007:34: error: unknown warning group '-Wfoo-bar', ignored [-Werror,-Wunknown-pragmas]
#pragma clang diagnostic ignored "-Wfoo-bar"
````

So: an alternative solution would just be to add "-Wno-shadow-field" in the moz.build here for the whole directory -- IIRC old clang versions are happier with unrecognized warnings there [maybe due to extra things we pass on the command line to make it happy].
Attachment #8847321 - Flags: review?(dholbert) → review-
For posterity, here's our discussion on irc:

16:22 dholbert | keeler: hi! I believe I can get around that warning by adding to the CLANG_CXX warning list section in moz.build.  (which seems to be friendlier to old clang versions, as I noted on the bug)
16:23 dholbert | keeler: I'm happy to steal the bug and post a patch that does that, but I don't want to step on your toes now that you've started on it -- happy to leave that in your hands too
16:23   keeler | dholbert: ok. The reason I was going with the pragma push/pop method was to limit it to just that declaration
16:24 dholbert | keeler: yeah, #pragma is strictly better from that perspective
16:24   keeler | would it be possible to add something like && __clang_major__ == whatever to match the specific clang version?
16:24 dholbert | keeler: I think that is possible, yes
16:24   keeler | dholbert: ok - cool. I was going to try that but I didn't know exactly what version to put. 4? 5?
16:25 dholbert | keeler: (make sure >= not ==, too -- presumably this warning will continue to exist going forward)
16:25   keeler | good call
16:25 dholbert | keeler: my clang trunk build reports itself as "5.0"
16:26 dholbert | keeler: though I suspect this warning may have been introduced in an older version
16:26   keeler | hmmm - unfortunately I can't find any documentation on just "Wshadow-field"
16:26 dholbert | ha! looks like it was proposed by ehsan: http://clang-developers.42468.n3.nabble.com/Proposal-Wshadow-field-flag-td4045024.html
16:26          | [urlgrab] loading http://clang-developers.42468.n3.nabble.com/Proposal-Wshadow-field-flag-td4045024.html locally
16:28   keeler | I wonder if there's a way to test for if a warning exists in clang? :)
16:40 dholbert | keeler: https://groups.google.com/a/skia.org/forum/#!msg/reviews/gInJ6loP840/0beVpycYDQAJ suggests that clang 4.0 supports this warning
16:40          | [urlgrab] loading https://groups.google.com/a/skia.org/forum/#!msg/reviews/gInJ6loP840/0beVpycYDQAJ locally
16:40 dholbert | (tangentially -- there's a mention of an upcoming builder upgrade to clang 4.0 in the last post there, after some discussion of fixing this warning in their code)
16:41   keeler | dholbert: the second to last comment seems to be saying it's new in 5?
16:41 dholbert | oh, right
16:41 dholbert | Ah! "shadow-all" might be a blanket warning group that covers this, based on looking at clang source...
16:43   keeler | well, let's start with "Wshadow-field" for __clang_major__ >= 5 for now, and we can fix it if anyone has any issues with 4
16:43 dholbert | keeler: that works, though your first patch with s/shadow/shadow-all/ WFM as well
16:43 dholbert | (just tested)
16:44   keeler | ok - thanks. I think I'd prefer the more specific approach, though
16:44 dholbert | (that does disable a few other related compiler warnings in the crossfire of course)
16:44 dholbert | keeler: cool! that's wise. I'm a bit jaded about targeting with this stuff. :)
16:45   keeler | yeah, I can see how being too specific might be a pain in the long run :
16:45   keeler | :/
Comment on attachment 8847321 [details]
bug 1339921 - disable clang's shadowed field warning in a mozilla::pkix gtest class

https://reviewboard.mozilla.org/r/120316/#review122276

This looks sane to me, and I verified that it suppresses this warning/error in my build with clang trunk [which reports itself as version 5.0].

r=me
Attachment #8847321 - Flags: review?(dholbert) → review+
Comment on attachment 8847321 [details]
bug 1339921 - disable clang's shadowed field warning in a mozilla::pkix gtest class

https://reviewboard.mozilla.org/r/120316/#review122574

This looks fine, but I believe we actually can test whether or not the clang being used supports a particular warning:
```
#if defined(__clang__) && __has_warning("-Wshadow-field")
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wshadow-field"
```
(See https://clang.llvm.org/docs/LanguageExtensions.html#has-warning for trunk/5.0.)
Our minimum clang version is 3.6, which supports this macro as well: http://releases.llvm.org/3.6.0/tools/clang/docs/LanguageExtensions.html#has-warning.
I'm going to assume this means [3.6, trunk] all support this macro.

r=me for either of these two solutions.
Attachment #8847321 - Flags: review?(cykesiopka.bmo) → review+
Ah, *that's* the kind of thing I was looking for. Thanks!
:dholbert, can you confirm this works for you?
Flags: needinfo?(dholbert)
Awesome - I didn't know that clang feature existed!  I've confirmed that this works for me (and I also confirmed that nothing explodes if I use an unrecognized warning like "-Wfoo-bar" in this syntax).

So I think this is good to go.
Flags: needinfo?(dholbert) → needinfo?(dkeeler)
Unfortunately, as you already know, software: apparently gcc can't parse the '__has_warning("-Wshadow-field")' part of '#if defined(__clang__) && __has_warning("-Wshadow-field")' even if the whole expression would be false, so I had to break up the conjunction into two statements. So, this now looks a little bulky, but at least it's precise and gcc accepts it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4333c58ee5d5f9b925f38320f80c228daf28564b
Flags: needinfo?(dkeeler)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ba79bf5e771
disable clang's shadowed field warning in a mozilla::pkix gtest class r=Cykesiopka,dholbert
https://hg.mozilla.org/mozilla-central/rev/9ba79bf5e771
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attachment #8863266 - Flags: review?(kbrosnan)
Attachment #8863266 - Attachment is obsolete: true
Attachment #8863266 - Flags: review?(kbrosnan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: