This is an archive of the discontinued LLVM Phabricator instance.

Paths

Table of Contentst

-
llvm/trunk/utils/benchmark/
-
trunk/
-
utils/
-
benchmark/
-
CMakeLists.txt
-
README.LLVM

Differential D52998

[benchmark] Disable exceptions in Microsoft STL
ClosedPublic

Authored by eandrews on Oct 8 2018, 1:54 PM.

Download Raw Diff

Details

Reviewers

rnk
kbobyrev
lebedev.ri
erichkeane

Commits

rGb3fbba6d15d3: [benchmark] Disable exceptions in Microsoft STL
rL346237: [benchmark] Disable exceptions in Microsoft STL

Summary

Define _HAS_EXCEPTIONS=0, when compiling benchmark files, to disable exceptions in Microsoft STL.

Windows builds were failing due to C4530 warnings (C++ exception handler used, but unwind semantics are not enabled) thrown by MSVC standard library.

Diff Detail

Repository: rL LLVM

Event Timeline

eandrews created this revision.Oct 8 2018, 1:54 PM

Herald added a subscriber: mgorny. · View Herald TranscriptOct 8 2018, 1:54 PM

Are those warnings about C++ exceptions, or some windows-specific exception stuff?
It seems the C++ exceptions are used in tests e.g.
https://github.com/google/benchmark/search?q=throw&unscoped_q=throw
https://github.com/google/benchmark/search?q=catch&unscoped_q=catch

I do not think defining _HAS_EXCEPTIONS=0 affects C++ exceptions in the application. I thought it only affected the STL. I will verify this and update you.

_HAS_EXCEPTIONS=0 is an undocumented STL specific thing that the library
implementation uses to mean "don't write code that does throw X;, do
something else instead".

Thanks for the information Zachary.

@lebedev.ri I do not know how benchmark library developers need/want to handle exceptions in MSVC STL. If these need to be enabled when BENCHMARK_ENABLE_EXCEPTIONS is true, I think we can just modify _HAS_EXCEPTIONS in utils/benchmark/CMakeLists.txt.

It's not enough to just set _HAS_EXCEPTIONS=1, it has to match whatever the value of /EH is passed to the compiler. So if we need to be able to throw catch exceptions, then you should pass /EHsc and not touch _HAS_EXCEPTIONS (technically you could set it to 1 but that's the default). And if we don't need to be able to throw or catch exceptions, then we pass /EHs-c- (which is what we do today) and also set _HAS_EXCEPTIONS=0. But the two should agree with each other.

Yes. I understand. At the moment, exception handling flags are generated based on BENCHMARK_ENABLE_EXCEPTIONS in utils/benchmark/CMakeLists.txt . However, _HAS_EXCEPTIONS is not defined based on this (code below). The warnings are a result of this mismatch.

if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
    add_cxx_compiler_flag(-EHs-)
    add_cxx_compiler_flag(-EHa-)
  endif()

I think it makes most sense to add definition for _HAS_EXCEPTIONS=0 here as opposed to modifying llvm/CMakeLists.txt. Please correct me if I'm wrong. I'm not too familiar with CMake. @kbobyrev Please let me know what you think as well since you had suggested llvm/CMakeLists.txt.

In D52998#1258962, @eandrews wrote:
Yes. I understand. At the moment, exception handling flags are generated based on BENCHMARK_ENABLE_EXCEPTIONS in utils/benchmark/CMakeLists.txt . However, _HAS_EXCEPTIONS is not defined based on this (code below). The warnings are a result of this mismatch.
if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
    add_cxx_compiler_flag(-EHs-)
    add_cxx_compiler_flag(-EHa-)
  endif()
I think it makes most sense to add definition for _HAS_EXCEPTIONS=0 here as opposed to modifying llvm/CMakeLists.txt.

Then i'd recommend/suggest to submit this upstream first.

Please correct me if I'm wrong. I'm not too familiar with CMake. @kbobyrev Please let me know what you think as well since you had suggested llvm/CMakeLists.txt.

In D52998#1259602, @lebedev.ri wrote:
In D52998#1258962, @eandrews wrote:
Yes. I understand. At the moment, exception handling flags are generated based on BENCHMARK_ENABLE_EXCEPTIONS in utils/benchmark/CMakeLists.txt . However, _HAS_EXCEPTIONS is not defined based on this (code below). The warnings are a result of this mismatch.
if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
    add_cxx_compiler_flag(-EHs-)
    add_cxx_compiler_flag(-EHa-)
  endif()
I think it makes most sense to add definition for _HAS_EXCEPTIONS=0 here as opposed to modifying llvm/CMakeLists.txt.
Then i'd recommend/suggest to submit this upstream first.

Please correct me if I'm wrong. I'm not too familiar with CMake. @kbobyrev Please let me know what you think as well since you had suggested llvm/CMakeLists.txt.

@kbobyrev Is this alright with you?

Sorry for the delay; yes, this looks good to me!

This revision is now accepted and ready to land.Nov 1 2018, 2:56 AM

@kbobyrev I apologize if I was unclear in the comments. I was asking if the changes proposed in the comments are alright with you since they would involve modifying benchmark/CMakelists.txt (instead of llvm/CMakeLists.txt as discussed in mailing list). As Zachary mentioned in comments, _HAS_EXCEPTIONS should be set to 0 only when exceptions are disabled. Since exception handling for benchmarks is handled in benchmark/CMakeLists.txt, I think it makes most sense to add the definition there. I have now uploaded the proposed change for review.

I am still working with my company to figure out the corporate CLA Google's benchmark project requires for patch submissions. I can submit the patch upstream once that is done. If you would prefer to submit the patch upstream yourself, please feel free to do so.

Sorry again for the confusion!

In D52998#1284146, @eandrews wrote:

@kbobyrev I apologize if I was unclear in the comments. I was asking if the changes proposed in the comments are alright with you since they would involve modifying benchmark/CMakelists.txt (instead of llvm/CMakeLists.txt as discussed in mailing list). As Zachary mentioned in comments, _HAS_EXCEPTIONS should be set to 0 only when exceptions are disabled. Since exception handling for benchmarks is handled in benchmark/CMakeLists.txt, I think it makes most sense to add the definition there. I have now uploaded the proposed change for review.

I am still working with my company to figure out the corporate CLA Google's benchmark project requires for patch submissions. I can submit the patch upstream once that is done. If you would prefer to submit the patch upstream yourself, please feel free to do so.

Sorry again for the confusion!

Ah, my bad, I didn't notice it's the utils/benchmark/CMakeLists.txt :( Apologies.

Upstreaming it first might be better, especially since the change seems to be trivial. Is this line addition the only change proposed for the benchmark library? If so, I could submit a PR and probably get it accepted within the next few days. Then this patch would simply add commit hash and small notice to utils/benchmark/README.LLVM and that would be it.

Do you want me to submit the patch to the benchmark library? It seems that it would help to speedup the process.

This revision now requires changes to proceed.Nov 1 2018, 10:57 AM

Upstreaming it first might be better, especially since the change seems to be trivial. Is this line addition the only change proposed for the benchmark library?

Yes this line is the only change.

Do you want me to submit the patch to the benchmark library? It seems that it would help to speedup the process.

That would be helpful. Thank you!

In D52998#1284312, @eandrews wrote:

Upstreaming it first might be better, especially since the change seems to be trivial. Is this line addition the only change proposed for the benchmark library?

Yes this line is the only change.

Do you want me to submit the patch to the benchmark library? It seems that it would help to speedup the process.

That would be helpful. Thank you!

Opened a PR https://github.com/google/benchmark/pull/715.

kbobyrev removed a reviewer: omtcyfz.Nov 2 2018, 3:50 AM

The patch is good to go!

Please add the commit reference to utils/benchmark/README.LLVM (https://github.com/google/benchmark/commit/a9b31c51b1ee7ec7b31438c647123c2cbac5d956) and the patch could be landed!

This revision is now accepted and ready to land.Nov 2 2018, 9:30 AM

Thanks!

Closed by commit rL346237: [benchmark] Disable exceptions in Microsoft STL (authored by eandrews). · Explain WhyNov 6 2018, 8:00 AM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: llvm-commits. · View Herald TranscriptNov 6 2018, 8:00 AM

Revision Contents

Path

Size

llvm/

trunk/

utils/

benchmark/

CMakeLists.txt

1 line

README.LLVM

2 lines

Diff 172769

llvm/trunk/utils/benchmark/CMakeLists.txt

Show First 20 Lines • Show All 93 Lines • ▼ Show 20 Lines	if (MSVC)
# Turn compiler warnings up to 11		# Turn compiler warnings up to 11
string(REGEX REPLACE "[-/]W[1-4]" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")		string(REGEX REPLACE "[-/]W[1-4]" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4")		set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4")
add_definitions(-D_CRT_SECURE_NO_WARNINGS)		add_definitions(-D_CRT_SECURE_NO_WARNINGS)

if (NOT BENCHMARK_ENABLE_EXCEPTIONS)		if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
add_cxx_compiler_flag(-EHs-)		add_cxx_compiler_flag(-EHs-)
add_cxx_compiler_flag(-EHa-)		add_cxx_compiler_flag(-EHa-)
		add_definitions(-D_HAS_EXCEPTIONS=0)
endif()		endif()
# Link time optimisation		# Link time optimisation
if (BENCHMARK_ENABLE_LTO)		if (BENCHMARK_ENABLE_LTO)
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /GL")		set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /GL")
set(CMAKE_STATIC_LINKER_FLAGS_RELEASE "${CMAKE_STATIC_LINKER_FLAGS_RELEASE} /LTCG")		set(CMAKE_STATIC_LINKER_FLAGS_RELEASE "${CMAKE_STATIC_LINKER_FLAGS_RELEASE} /LTCG")
set(CMAKE_SHARED_LINKER_FLAGS_RELEASE "${CMAKE_SHARED_LINKER_FLAGS_RELEASE} /LTCG")		set(CMAKE_SHARED_LINKER_FLAGS_RELEASE "${CMAKE_SHARED_LINKER_FLAGS_RELEASE} /LTCG")
set(CMAKE_EXE_LINKER_FLAGS_RELEASE "${CMAKE_EXE_LINKER_FLAGS_RELEASE} /LTCG")		set(CMAKE_EXE_LINKER_FLAGS_RELEASE "${CMAKE_EXE_LINKER_FLAGS_RELEASE} /LTCG")

▲ Show 20 Lines • Show All 143 Lines • Show Last 20 Lines

llvm/trunk/utils/benchmark/README.LLVM

	Show All 13 Lines
	* https://github.com/google/benchmark/commit/505be96ab23056580a3a2315abba048f4428b04e			* https://github.com/google/benchmark/commit/505be96ab23056580a3a2315abba048f4428b04e
	is applied to comply with the LLVM's required CMake version			is applied to comply with the LLVM's required CMake version
	* https://github.com/google/benchmark/commit/f0901417c89d123474e6b91365029cfe32cf89dc			* https://github.com/google/benchmark/commit/f0901417c89d123474e6b91365029cfe32cf89dc
	is applied to fix 32-bit build failure on macOS			is applied to fix 32-bit build failure on macOS
	* https://github.com/google/benchmark/commit/52613079824ac58d06c070aa9fbbb186a5859e2c			* https://github.com/google/benchmark/commit/52613079824ac58d06c070aa9fbbb186a5859e2c
	is applied to fix cross compilation with MinGW headers			is applied to fix cross compilation with MinGW headers
	* https://github.com/google/benchmark/commit/439d6b1c2a6da5cb6adc4c4dfc555af235722396			* https://github.com/google/benchmark/commit/439d6b1c2a6da5cb6adc4c4dfc555af235722396
	is applied to fix building with MinGW headers for ARM			is applied to fix building with MinGW headers for ARM
				* https://github.com/google/benchmark/commit/a9b31c51b1ee7ec7b31438c647123c2cbac5d956
				is applied to disable exceptions in Microsoft STL when exceptions are disabled