1339921 - 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]

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

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").

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.)

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1339921 - disable clang's shadowed field warning in a mozilla::pkix gtest class

pkixocsp_VerifyEncodedResponse_GetCertTrust has a field trustDomain that
deliberately shadows the field it inherits from so that code doesn't use it by
accident.

Review commit: https://reviewboard.mozilla.org/r/120316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120316/

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.)

Comment 5

[Daniel Holbert [:dholbert]]

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].

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8847321 [details]
bug 1339921 - disable clang's shadowed field warning in a mozilla::pkix gtest class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120316/diff/1-2/

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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 8

[Daniel Holbert [:dholbert]]

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

Comment 9

[:Cykesiopka]

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.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8847321 [details]
bug 1339921 - disable clang's shadowed field warning in a mozilla::pkix gtest class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120316/diff/2-3/

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Ah, *that's* the kind of thing I was looking for. Thanks!
:dholbert, can you confirm this works for you?

Comment 12

[Daniel Holbert [: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.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8847321 [details]
bug 1339921 - disable clang's shadowed field warning in a mozilla::pkix gtest class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120316/diff/3-4/

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ba79bf5e771
disable clang's shadowed field warning in a mozilla::pkix gtest class r=Cykesiopka,dholbert

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ba79bf5e771

Comment 17

[friedger]

Attachment: Bug 1339921 - Clicking the "New Tab" icon from the widget should always show the newly opened tab

Add check for open tabs panel when opening url from external app

Review commit: https://reviewboard.mozilla.org/r/135048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135048/

Comment 18

[friedger]

Comment on attachment 8863266 [details]
Bug 1339921 - Clicking the "New Tab" icon from the widget should always show the newly opened tab

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135048/diff/1-2/