Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2336253002:
Fix clang-cl release component build failure on windows. (Closed)
DescriptionFix clang-cl release component build failure on windows.
Issue was caused in change:
https://codereview.chromium.org/2290733002
BUG=597963
Committed: https://crrev.com/705c8eb7fa3ee2ee19da9c78fdd8266108ae207e
Cr-Commit-Position: refs/heads/master@{#418267}
Patch Set 1 #
Messages
Total messages: 16 (6 generated)
Description was changed from ========== Fix clang-cl release build failure on windows. Issue was caused in change: https://codereview.chromium.org/2290733002 BUG=597963 ========== to ========== Fix clang-cl release build failure on windows. Issue was caused in change: https://codereview.chromium.org/2290733002 BUG=597963 ==========
dtapuska@chromium.org changed reviewers: + rbyers@chromium.org, thakis@chromium.org
Description was changed from ========== Fix clang-cl release build failure on windows. Issue was caused in change: https://codereview.chromium.org/2290733002 BUG=597963 ========== to ========== Fix clang-cl release component build failure on windows. Issue was caused in change: https://codereview.chromium.org/2290733002 BUG=597963 ==========
lgtm, thanks. I checked that msvc component builds don't need this. hans: The link error is here: https://codereview.chromium.org/2290733002/#msg24 (and UseCounter.h in that Cl also shows the code). I guess msvc decides to not inline the dtor of the outer class in the test, and so it doesn't need the dtor of the inner class in the test file -- so this only happens by accident in msvc as far as I understand (based on an inlining decision).
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Sorry I missed this. Nico, is there somewhere we should track build issues that get past the CQ? I.e. is this a rare isolated event not to worry about, or one of a pattern that suggests we need to add another bot to the CQ? LGTM
On 2016/09/13 15:08:40, Rick Byers wrote: > Thanks! Sorry I missed this. Nico, is there somewhere we should track build > issues that get past the CQ? I.e. is this a rare isolated event not to worry > about, or one of a pattern that suggests we need to add another bot to the CQ? Release component only build breakages are pretty rare (maybe twice a year?).
Message was sent while issue was closed.
Description was changed from ========== Fix clang-cl release component build failure on windows. Issue was caused in change: https://codereview.chromium.org/2290733002 BUG=597963 ========== to ========== Fix clang-cl release component build failure on windows. Issue was caused in change: https://codereview.chromium.org/2290733002 BUG=597963 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix clang-cl release component build failure on windows. Issue was caused in change: https://codereview.chromium.org/2290733002 BUG=597963 ========== to ========== Fix clang-cl release component build failure on windows. Issue was caused in change: https://codereview.chromium.org/2290733002 BUG=597963 Committed: https://crrev.com/705c8eb7fa3ee2ee19da9c78fdd8266108ae207e Cr-Commit-Position: refs/heads/master@{#418267} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/705c8eb7fa3ee2ee19da9c78fdd8266108ae207e Cr-Commit-Position: refs/heads/master@{#418267}
Message was sent while issue was closed.
On 2016/09/13 15:06:11, Nico wrote: > hans: The link error is here: https://codereview.chromium.org/2290733002/#msg24 > (and UseCounter.h in that Cl also shows the code). I guess msvc decides to not > inline the dtor of the outer class in the test, and so it doesn't need the dtor > of the inner class in the test file -- so this only happens by accident in msvc > as far as I understand (based on an inlining decision). Yes, that's what's happened here. Actually, clang-cl does try to avoid this situation after r246338, which is why we've seen less of it. It tries to check if a dllimport function is safe to inline based on whether that in turn references non-dllimport functions. However in this case it doesn't realize that ~UseCounter() calls ~LegacyUseCounter() because that call doesn't show up in the AST walk.
Message was sent while issue was closed.
On 2016/09/13 16:41:27, hans wrote: > On 2016/09/13 15:06:11, Nico wrote: > > hans: The link error is here: > https://codereview.chromium.org/2290733002/#msg24 > > (and UseCounter.h in that Cl also shows the code). I guess msvc decides to not > > inline the dtor of the outer class in the test, and so it doesn't need the > dtor > > of the inner class in the test file -- so this only happens by accident in > msvc > > as far as I understand (based on an inlining decision). > > Yes, that's what's happened here. > > Actually, clang-cl does try to avoid this situation after r246338, which is why > we've seen less of it. It tries to check if a dllimport function is safe to > inline based on whether that in turn references non-dllimport functions. However > in this case it doesn't realize that ~UseCounter() calls ~LegacyUseCounter() > because that call doesn't show up in the AST walk. Clang r281395 should make this better.
Message was sent while issue was closed.
On 2016/09/13 21:30:40, hans wrote: > On 2016/09/13 16:41:27, hans wrote: > > On 2016/09/13 15:06:11, Nico wrote: > > > hans: The link error is here: > > https://codereview.chromium.org/2290733002/#msg24 > > > (and UseCounter.h in that Cl also shows the code). I guess msvc decides to > not > > > inline the dtor of the outer class in the test, and so it doesn't need the > > dtor > > > of the inner class in the test file -- so this only happens by accident in > > msvc > > > as far as I understand (based on an inlining decision). > > > > Yes, that's what's happened here. > > > > Actually, clang-cl does try to avoid this situation after r246338, which is > why > > we've seen less of it. It tries to check if a dllimport function is safe to > > inline based on whether that in turn references non-dllimport functions. > However > > in this case it doesn't realize that ~UseCounter() calls ~LegacyUseCounter() > > because that call doesn't show up in the AST walk. > > Clang r281395 should make this better. Cool, thanks for the follow-through! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
