Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix x64/Debug build on MSVS #2

Merged
merged 1 commit into from
Mar 18, 2015
Merged

Fix x64/Debug build on MSVS #2

merged 1 commit into from
Mar 18, 2015

Conversation

tbennun
Copy link
Contributor

@tbennun tbennun commented Mar 13, 2015

This fixes building glog on Windows using the Debug configuration on the x64 platform.

@pphaneuf
Copy link
Contributor

I cannot find a CLA (contribution license agreement) on file for you? We cannot accept your contribution without one, unfortunately...

Could you make sure you have one by going here?

https://cla.developers.google.com/

Thanks!

@tbennun
Copy link
Contributor Author

tbennun commented Mar 13, 2015

@pphaneuf I submitted it just now.

@pphaneuf
Copy link
Contributor

And the newfangled CLA robot saw it. Thanks!

I'm not reviewing the code change, I have no idea about this Windows stuff, just dropping by for the CLA stuff, leaving it to the other maintainers to merge this in.

@@ -1459,7 +1459,7 @@ static void logging_fail() {
#if defined(_DEBUG) && defined(_MSC_VER)
// When debugging on windows, avoid the obnoxious dialog and make
// it possible to continue past a LOG(FATAL) in the debugger
_asm int 3
__debugbreak();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It compiles without it.
As far as I know, <windows.h> includes it, so maybe windows/port.h is included somehow.

To be sure, the function "DebugBreak()" from windows.h can be used:
https://msdn.microsoft.com/library/windows/desktop/ms679297.aspx

However, the official replacement (according to https://msdn.microsoft.com/en-us/library/45yd4tzz.aspx) for _asm int 3 is __debugbreak().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.
lgtm

@ukai ukai added the LGTM label Mar 18, 2015
ukai added a commit that referenced this pull request Mar 18, 2015
Fix x64/Debug build on MSVS
@ukai ukai merged commit 47ab571 into google:master Mar 18, 2015
kennyyu added a commit to kennyyu/glog that referenced this pull request Nov 4, 2017
Summary:
Issue google#80 points out several places in glog where TSAN discovers
false positives. One of these places is in the `LOG_EVERY_N` macros.
These macros are implemented by maintaining a static unprotected
integer counter, and TSAN will report data races on these counters.

Here is a minimum example to reproduce the data race:

```

void logging() {
  for (int i = 0; i < 300; ++i) {
    LOG_EVERY_N(INFO, 2) << "foo";
  }
}

int main() {
  auto t1 = std::thread(logging);
  auto t2 = std::thread(logging);
  t1.join();
  t2.join();
  return 0;
}
```

And here is the TSAN report:

```
WARNING: ThreadSanitizer: data race (pid=776850)
  Write of size 4 at 0x558de483f684 by thread T2:
    #0 logging()
    google#1 void std::_Bind_simple<void (*())()>::_M_invoke<>(std::_Index_tuple<>)
    google#2 std::_Bind_simple<void (*())()>::operator()()
    google#3 std::thread::_Impl<std::_Bind_simple<void (*())()> >::_M_run()
    google#4 execute_native_thread_routine

  Previous write of size 4 at 0x558de483f684 by thread T1:
    #0 logging()
    google#1 void std::_Bind_simple<void (*())()>::_M_invoke<>(std::_Index_tuple<>)
    google#2 std::_Bind_simple<void (*())()>::operator()()
    google#3 std::thread::_Impl<std::_Bind_simple<void (*())()> >::_M_run()
    google#4 execute_native_thread_routine

  Location is global '<null>' at 0x000000000000 (main+0x00000011c684)

  Thread T2 (tid=776857, running) created by main thread at:
    #0 pthread_create
    google#1 __gthread_create
    google#2 std::thread::_M_start_thread(std::shared_ptr<std::thread::_Impl_base>, void (*)())
    google#3 main

  Thread T1 (tid=776856, running) created by main thread at:
    #0 pthread_create
    google#1 __gthread_create
    google#2 std::thread::_M_start_thread(std::shared_ptr<std::thread::_Impl_base>, void (*)())
    google#3 main

SUMMARY: ThreadSanitizer: data race in logging()
```

To avoid noisy TSAN reports and also avoid adding a performance hit, this
change will mark these counters as benign races so that TSAN will not report
them. This change will only have an effect if we are compiling with TSAN;
there are no changes if we are not building with TSAN.

With this change, the above example no longer reports a data race when built
and run with TSAN.
durswd pushed a commit to durswd/glog that referenced this pull request Sep 2, 2019
Fix x64/Debug build on MSVS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants