|
|
Created:
4 years, 9 months ago by brucedawson Modified:
4 years, 9 months ago CC:
chromium-reviews, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix VS 2015 Update 2 specific bug
When building Chrome with VS 2015 Update 2 RC there is a compiler error:
1> texture_definition.cc
1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage'
1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage'
1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)'
This is caused by a compiler bug, together with a type trait used in
std::vector:
https://twitter.com/StephanTLavavej/status/710191543840219136
A VS bug has been filed at:
https://connect.microsoft.com/VisualStudio/feedback/details/2475971
Bug 595189 has a more minimal repro.
BUG=440500, 595189
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/4d9ecc15b1ca04d30113408a4d2c428456176ce0
Cr-Commit-Position: refs/heads/master@{#381640}
Patch Set 1 #Patch Set 2 : More tightly scoped fix. #Patch Set 3 : Added comment #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== Fix VS 2015 Update 2 RC specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This does not exist in VS 2013, other versions of VS 2015, gcc, or clang. Therefore this fix is definitely a hack, but it seems appropriate to unblock external developers who are building with VS 2015 Update 2 RC. A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 BUG=440500 ========== to ========== Fix VS 2015 Update 2 RC specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This does not exist in VS 2013, other versions of VS 2015, gcc, or clang. Therefore this fix is definitely a hack, but it seems appropriate to unblock external developers who are building with VS 2015 Update 2 RC. A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 BUG=440500 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
brucedawson@chromium.org changed reviewers: + piman@chromium.org
This works around an odd compiler bug that a few people are hitting. This would probably be a temporary workaround, if Microsoft fixes the bug. Any thoughts?
Description was changed from ========== Fix VS 2015 Update 2 RC specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This does not exist in VS 2013, other versions of VS 2015, gcc, or clang. Therefore this fix is definitely a hack, but it seems appropriate to unblock external developers who are building with VS 2015 Update 2 RC. A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 BUG=440500 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Fix VS 2015 Update 2 RC specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This does not exist in VS 2013, other versions of VS 2015, gcc, or clang. Therefore this fix is definitely a hack, but it seems appropriate to unblock external developers who are building with VS 2015 Update 2 RC. A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro and explanation. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Description was changed from ========== Fix VS 2015 Update 2 RC specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This does not exist in VS 2013, other versions of VS 2015, gcc, or clang. Therefore this fix is definitely a hack, but it seems appropriate to unblock external developers who are building with VS 2015 Update 2 RC. A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro and explanation. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Fix VS 2015 Update 2 RC specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This does not exist in VS 2013, other versions of VS 2015, gcc, or clang. Therefore this fix is definitely a hack, but it seems appropriate to unblock external developers who are building with VS 2015 Update 2 RC. A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro and explanation of the underlying issue which is a change in std::vector. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Description was changed from ========== Fix VS 2015 Update 2 RC specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This does not exist in VS 2013, other versions of VS 2015, gcc, or clang. Therefore this fix is definitely a hack, but it seems appropriate to unblock external developers who are building with VS 2015 Update 2 RC. A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro and explanation of the underlying issue which is a change in std::vector. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Fix VS 2015 Update 2 specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This does not exist in VS 2013, other versions of VS 2015, gcc, or clang. Therefore this fix is definitely a hack, but it seems appropriate to unblock external developers who are building with VS 2015 Update 2 RC, and may be needed with Update 2 final. A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro and explanation of the underlying issue which is a change in std::vector. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
brucedawson@chromium.org changed reviewers: + zmo@chromium.org
I've tidied up this fix a bit and I think it's ready to land now. PTAL? I also now understand the problem better - it's a change to one of the VS 2015 header files - probably <vector> - that triggers the compile error.
zmo@chromium.org changed reviewers: + geofflang@chromium.org, jmadill@chromium.org
On 2016/03/16 18:31:14, brucedawson wrote: > I've tidied up this fix a bit and I think it's ready to land now. PTAL? > > I also now understand the problem better - it's a change to one of the VS 2015 > header files - probably <vector> - that triggers the compile error. Adding jmadill/geofflang, they know vs much better Still, can you explain a little deeper why these extra includes workaround the bug?
The compile error occurs because the Update 2 version of vector causes the definition of GLStreamTextureImage to be required, for some reason. This fix works by making the definition available.
On 2016/03/16 19:23:07, brucedawson wrote: > The compile error occurs because the Update 2 version of vector causes the > definition of GLStreamTextureImage to be required, for some reason. This fix > works by making the definition available. But why in texture_definition.cc? I don't see how this file is connected to GLStreamTextureImage. Also, why do you need gpu_export.h included in gl_stream_texture_image.h? Please help me fully understand. I agree we should enable developers using VS2015, but we also want to make sure the fix is accurate and right to the root and as isolated as possible.
> But why in texture_definition.cc? I don't see how this file is connected to > GLStreamTextureImage. Ah - I understand the question now. That information is in the linked bugs, but isn't very easy to find. The include is needed in that file because that is the one file that refused to compile. The problem is triggered by line 394, which is: texture->face_infos_.resize(1); This line means that a FaceInfo object could be created or destroyed. FaceInfo contains a vector of LevelInfo objects, and LevelInfo contains a scoped_refptr<GLStreamTextureImage>. None of this should actually require a definition for GLStreamTextureImage, but due to the compiler bug it does. > Also, why do you need gpu_export.h included in gl_stream_texture_image.h? The gpu_export.h include I am less certain about and would appreciate feedback from jmadill/geofflang. If I don't have that include then the compile fails because GPU_EXPORT is not defined. I could also include gpu_export.h from texture_definition.cc but IncludeWhatYouUse principles suggest that gl_stream_texture_image.h should be including gpu_export.h, since it relies on it. Thus, that include both makes this fix work and makes the code less fragile. > I agree we should enable developers using VS2015, but we also want to make sure > the fix is accurate and right to the root and as isolated as possible. Absolutely.
On Wed, Mar 16, 2016 at 1:33 PM, <brucedawson@chromium.org> wrote: > > But why in texture_definition.cc? I don't see how this file is connected > to > > GLStreamTextureImage. > > Ah - I understand the question now. That information is in the linked > bugs, but > isn't very easy to find. > > The include is needed in that file because that is the one file that > refused to > compile. The problem is triggered by line 394, which is: > > texture->face_infos_.resize(1); > > This line means that a FaceInfo object could be created or destroyed. > FaceInfo > contains a vector of LevelInfo objects, and LevelInfo contains a > scoped_refptr<GLStreamTextureImage>. None of this should actually require a > definition for GLStreamTextureImage, but due to the compiler bug it does. > Actually, can't resize() call the copy constructor for scoped_refptr<GLStreamTextureImage>, which calls AddRef/Release, which needs the definition? Either way, since it sounds like the bug happens due to the use of vector<scoped_refptr<GLStreamTextureImage>> in texture_manager.h, should the include be there, maybe with a comment about the bug/TODO to remove? > > > Also, why do you need gpu_export.h included in gl_stream_texture_image.h? > > The gpu_export.h include I am less certain about and would appreciate > feedback > from jmadill/geofflang. If I don't have that include then the compile fails > because GPU_EXPORT is not defined. I could also include gpu_export.h from > texture_definition.cc but IncludeWhatYouUse principles suggest that > gl_stream_texture_image.h should be including gpu_export.h, since it > relies on > it. Thus, that include both makes this fix work and makes the code less > fragile. > Yes, this is the correct approach here. > > > > I agree we should enable developers using VS2015, but we also want to > make > sure > > the fix is accurate and right to the root and as isolated as possible. > > Absolutely. > > > https://codereview.chromium.org/1806743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Actually, can't resize() call the copy constructor for > scoped_refptr<GLStreamTextureImage>, which calls AddRef/Release, which > needs the definition? It's not supposed to call the copy constructor - destructor or default constructor only - so it *shouldn't* need AddRef/Release. Microsoft confirmed that it is a compiler bug (not surprising since all other compilers accept the code). > Either way, since it sounds like the bug happens due to the use of > vector<scoped_refptr<GLStreamTextureImage>> in texture_manager.h, should > the include be there, maybe with a comment about the bug/TODO to remove? I could do that. In fact that was my first solution but I didn't like it because it felt inelegant to pull in the definition in a header file (thus pulling it in to many translation units) when only one translation unit needed it. I'm happy to do it either way. Good point about a comment on the gl_stream_texture_image.h with bug link and TODO removal. Let me know which way you prefer.
LGTM for this, if you can just add a comment as to why the include is there in texture_definition.cc
Description was changed from ========== Fix VS 2015 Update 2 specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This does not exist in VS 2013, other versions of VS 2015, gcc, or clang. Therefore this fix is definitely a hack, but it seems appropriate to unblock external developers who are building with VS 2015 Update 2 RC, and may be needed with Update 2 final. A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro and explanation of the underlying issue which is a change in std::vector. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Fix VS 2015 Update 2 specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This is caused by a compiler bug, together with a type trait used in std::vector: https://twitter.com/StephanTLavavej/status/710191543840219136 A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
On 2016/03/17 00:03:56, piman wrote: > LGTM for this, if you can just add a comment as to why the include is there in > texture_definition.cc Comment added. I also cleaned up the description now that the root cause is better understood.
lgtm
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1806743002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1806743002/40001
Message was sent while issue was closed.
Description was changed from ========== Fix VS 2015 Update 2 specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This is caused by a compiler bug, together with a type trait used in std::vector: https://twitter.com/StephanTLavavej/status/710191543840219136 A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Fix VS 2015 Update 2 specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This is caused by a compiler bug, together with a type trait used in std::vector: https://twitter.com/StephanTLavavej/status/710191543840219136 A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix VS 2015 Update 2 specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This is caused by a compiler bug, together with a type trait used in std::vector: https://twitter.com/StephanTLavavej/status/710191543840219136 A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Fix VS 2015 Update 2 specific bug When building Chrome with VS 2015 Update 2 RC there is a compiler error: 1> texture_definition.cc 1>base/memory/ref_counted.h(414): error C2027: use of undefined type 'gpu::gles2::GLStreamTextureImage' 1> gpu/command_buffer/service/texture_manager.h(31): note: see declaration of 'gpu::gles2::GLStreamTextureImage' 1> base/memory/ref_counted.h(413): note: while compiling class template member function 'void scoped_refptr<gpu::gles2::GLStreamTextureImage>::AddRef(T *)' This is caused by a compiler bug, together with a type trait used in std::vector: https://twitter.com/StephanTLavavej/status/710191543840219136 A VS bug has been filed at: https://connect.microsoft.com/VisualStudio/feedback/details/2475971 Bug 595189 has a more minimal repro. BUG=440500,595189 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/4d9ecc15b1ca04d30113408a4d2c428456176ce0 Cr-Commit-Position: refs/heads/master@{#381640} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4d9ecc15b1ca04d30113408a4d2c428456176ce0 Cr-Commit-Position: refs/heads/master@{#381640}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2710683014/ by brucedawson@chromium.org. The reason for reverting is: This was a compiler bug workaround and the bug should have been fixed long ago.. |