[MSVC] Silence -Wnon-virtual-dtor on DIA APIs

[aganea]

Summary

When building LLVM with Clang 13.0 on Windows & targetting MSVC, I see a bunch of -Wnon-virtual-dtor warnings on DIA (Debug Interface Access) COM APIs.

[hans]

clang/lib/Driver/ToolChains/MSVC.cpp

54 ↗

(On Diff #396312)

Since it's in our repository, would it be possible to fix clang/lib/Driver/ToolChains/MSVCSetupApi.h to be warning free? If it's not possible to fix the classes the warning refers to, I think it would at least be nicer to move the pragma into that file.

llvm/include/llvm/DebugInfo/PDB/DIA/DIASupport.h

34 ↗

(On Diff #396312)

I thought this warning should be suppressed for system headers. Isn't that working here?

[aganea]

As suggested by @hans

[aganea]

llvm/include/llvm/DebugInfo/PDB/DIA/DIASupport.h

34 ↗

(On Diff #396312)

Good point, because it wasn't included as a system header in the first place.

[hans]

lgtm

[aganea]

Thanks!

[aganea]

I was wondering, can we mandate LLVM_ENABLE_WERROR=ON for all bots at least? (or maybe enable it through cmake, if ever there's a setting to detect we're running as a bot)
Or just reverse the default setting on the main branch, ie. make LLVM_ENABLE_WERROR=ON by default and let users disable it locally. I think that would encourage developers to care more about warnings? Release branches could have it disabled somehow, to accommodate for the changing compiler environments. Was this discussed before?

[hans]

In D116313#3218224, @aganea wrote:

I was wondering, can we mandate LLVM_ENABLE_WERROR=ON for all bots at least? (or maybe enable it through cmake, if ever there's a setting to detect we're running as a bot)
Or just reverse the default setting on the main branch, ie. make LLVM_ENABLE_WERROR=ON by default and let users disable it locally. I think that would encourage developers to care more about warnings? Release branches could have it disabled somehow, to accommodate for the changing compiler environments. Was this discussed before?

I don't remember whether this was discussed before. I think it would be a good topic for llvm-dev.

It would be nice to have at least one buildbot using -Werror, with some specific blessed compiler version, but for that to not get annoying I think we'd need to surface those warnings in code reviews, and even if we have system a for that today it doesn't seem to be working very well.

[dblaikie]

In D116313#3222508, @hans wrote:

In D116313#3218224, @aganea wrote:

I was wondering, can we mandate LLVM_ENABLE_WERROR=ON for all bots at least? (or maybe enable it through cmake, if ever there's a setting to detect we're running as a bot)
Or just reverse the default setting on the main branch, ie. make LLVM_ENABLE_WERROR=ON by default and let users disable it locally. I think that would encourage developers to care more about warnings? Release branches could have it disabled somehow, to accommodate for the changing compiler environments. Was this discussed before?

I don't remember whether this was discussed before. I think it would be a good topic for llvm-dev.

It would be nice to have at least one buildbot using -Werror, with some specific blessed compiler version, but for that to not get annoying I think we'd need to surface those warnings in code reviews, and even if we have system a for that today it doesn't seem to be working very well.

Not sure we'd be much worse off than we are today if some buildbots enabled -Werror without phabricator pre-commit integration. I'm pretty sure we've had -Werror buildbots in the past in that situation. In some ways it'd be ideal if it were a bootstrap build so we ensure LLVM's always warning-clean with current Clang, but that means longer turnaround. (wonder if we could configure buildbots to fail on warnings, but not enable -Werror - so the buildbot would also still produce other useful results, rather than halting on the first warning & potentially hiding/delaying other useful findings)

clang/lib/Driver/ToolChains/MSVCSetupApi.h

 // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.
 // </license>
 
 #pragma once
 
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wnon-virtual-dtor"
+#endif
+
 // Constants
 //
 #ifndef E_NOTFOUND
 #define E_NOTFOUND HRESULT_FROM_WIN32(ERROR_NOT_FOUND)
 #endif
 
 #ifndef E_FILENOTFOUND
 #define E_FILENOTFOUND HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)

 /// <param name="pReserved">Reserved for future use.</param>
 /// <returns>Standard HRESULT indicating success or failure.</returns>
 STDMETHODIMP GetSetupConfiguration(_Out_ ISetupConfiguration **ppConfiguration,
                                    _Reserved_ LPVOID pReserved);
 
 #ifdef __cplusplus
 }
 #endif
+
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif

llvm/lib/DebugInfo/PDB/CMakeLists.txt

 macro(add_pdb_impl_folder group)
   list(APPEND PDB_IMPL_SOURCES ${ARGN})
   source_group(${group} FILES ${ARGN})
 endmacro()
 
 if(LLVM_ENABLE_DIA_SDK)
-  include_directories(${MSVC_DIA_SDK_DIR}/include)
+  include_directories(SYSTEM ${MSVC_DIA_SDK_DIR}/include)
   set(LIBPDB_LINK_FOLDERS "${MSVC_DIA_SDK_DIR}\\lib")
 
   if ("$ENV{VSCMD_ARG_TGT_ARCH}" STREQUAL "arm64")
     set(LIBPDB_LINK_FOLDERS "${LIBPDB_LINK_FOLDERS}\\arm64")
   elseif ("$ENV{VSCMD_ARG_TGT_ARCH}" STREQUAL "arm")
     set(LIBPDB_LINK_FOLDERS "${LIBPDB_LINK_FOLDERS}\\arm")
   elseif (CMAKE_SIZEOF_VOID_P EQUAL 8)
     set(LIBPDB_LINK_FOLDERS "${LIBPDB_LINK_FOLDERS}\\amd64")