[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346237: [benchmark] Disable exceptions in Microsoft STL 
(authored by eandrews, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52998?vs=172171&id=172769#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52998

Files:
  llvm/trunk/utils/benchmark/CMakeLists.txt
  llvm/trunk/utils/benchmark/README.LLVM


Index: llvm/trunk/utils/benchmark/CMakeLists.txt
===
--- llvm/trunk/utils/benchmark/CMakeLists.txt
+++ llvm/trunk/utils/benchmark/CMakeLists.txt
@@ -99,6 +99,7 @@
   if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
 add_cxx_compiler_flag(-EHs-)
 add_cxx_compiler_flag(-EHa-)
+add_definitions(-D_HAS_EXCEPTIONS=0)
   endif()
   # Link time optimisation
   if (BENCHMARK_ENABLE_LTO)
Index: llvm/trunk/utils/benchmark/README.LLVM
===
--- llvm/trunk/utils/benchmark/README.LLVM
+++ llvm/trunk/utils/benchmark/README.LLVM
@@ -19,3 +19,5 @@
   is applied to fix cross compilation with MinGW headers
 * 
https://github.com/google/benchmark/commit/439d6b1c2a6da5cb6adc4c4dfc555af235722396
   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


Index: llvm/trunk/utils/benchmark/CMakeLists.txt
===
--- llvm/trunk/utils/benchmark/CMakeLists.txt
+++ llvm/trunk/utils/benchmark/CMakeLists.txt
@@ -99,6 +99,7 @@
   if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
 add_cxx_compiler_flag(-EHs-)
 add_cxx_compiler_flag(-EHa-)
+add_definitions(-D_HAS_EXCEPTIONS=0)
   endif()
   # Link time optimisation
   if (BENCHMARK_ENABLE_LTO)
Index: llvm/trunk/utils/benchmark/README.LLVM
===
--- llvm/trunk/utils/benchmark/README.LLVM
+++ llvm/trunk/utils/benchmark/README.LLVM
@@ -19,3 +19,5 @@
   is applied to fix cross compilation with MinGW headers
 * https://github.com/google/benchmark/commit/439d6b1c2a6da5cb6adc4c4dfc555af235722396
   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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

Thanks!


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev accepted this revision.
kbobyrev added a comment.
This revision is now accepted and ready to land.

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!


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

> 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!


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev requested changes to this revision.
kbobyrev added a comment.
This revision now requires changes to proceed.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews updated this revision to Diff 172171.
eandrews added a comment.

@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!


https://reviews.llvm.org/D52998

Files:
  utils/benchmark/CMakeLists.txt


Index: utils/benchmark/CMakeLists.txt
===
--- utils/benchmark/CMakeLists.txt
+++ utils/benchmark/CMakeLists.txt
@@ -99,6 +99,7 @@
   if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
 add_cxx_compiler_flag(-EHs-)
 add_cxx_compiler_flag(-EHa-)
+add_definitions(-D_HAS_EXCEPTIONS=0)
   endif()
   # Link time optimisation
   if (BENCHMARK_ENABLE_LTO)


Index: utils/benchmark/CMakeLists.txt
===
--- utils/benchmark/CMakeLists.txt
+++ utils/benchmark/CMakeLists.txt
@@ -99,6 +99,7 @@
   if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
 add_cxx_compiler_flag(-EHs-)
 add_cxx_compiler_flag(-EHa-)
+add_definitions(-D_HAS_EXCEPTIONS=0)
   endif()
   # Link time optimisation
   if (BENCHMARK_ENABLE_LTO)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev accepted this revision.
kbobyrev added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/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`.  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`.


As discussed in the mailing list, I think it also makes sense to set this

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


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

In https://reviews.llvm.org/D52998#1259602, @lebedev.ri wrote:

> In https://reviews.llvm.org/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?


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/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`.


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

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


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

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.


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

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.


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Zachary Turner via Phabricator via cfe-commits]

zturner added subscribers: eandrews, zturner.
zturner added a comment.

`_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".


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Zachary Turner via cfe-commits]

`_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".

On Mon, Oct 8, 2018 at 2:27 PM Elizabeth Andrews via Phabricator <
revi...@reviews.llvm.org> wrote:

> eandrews added a comment.
>
> 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.
>
>
> https://reviews.llvm.org/D52998
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

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.


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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


https://reviews.llvm.org/D52998



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews created this revision.
eandrews added reviewers: rnk, kbobyrev, omtcyfz, lebedev.ri, erichkeane.
Herald added a subscriber: mgorny.

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.


https://reviews.llvm.org/D52998

Files:
  CMakeLists.txt


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -1089,7 +1089,8 @@
   set(BENCHMARK_ENABLE_GTEST_TESTS OFF CACHE BOOL "Disable Google Test in 
benchmark" FORCE)
   # Since LLVM requires C++11 it is safe to assume that std::regex is 
available.
   set(HAVE_STD_REGEX ON CACHE BOOL "OK" FORCE)
-
+  # Disable exceptions in Microsoft STL to prevent warnings about exception 
handlers in STL.
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_HAS_EXCEPTIONS=0")
   add_subdirectory(utils/benchmark)
   add_subdirectory(benchmarks)
 endif()


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -1089,7 +1089,8 @@
   set(BENCHMARK_ENABLE_GTEST_TESTS OFF CACHE BOOL "Disable Google Test in benchmark" FORCE)
   # Since LLVM requires C++11 it is safe to assume that std::regex is available.
   set(HAVE_STD_REGEX ON CACHE BOOL "OK" FORCE)
-
+  # Disable exceptions in Microsoft STL to prevent warnings about exception handlers in STL.
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_HAS_EXCEPTIONS=0")
   add_subdirectory(utils/benchmark)
   add_subdirectory(benchmarks)
 endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits