Fix broken stacktrace

[marekcirkos]

Since #846, HAVE_UNWIND_H is not really in use. Instead we should use HAVE__UNWIND_BACKTRACE and HAVE__UNWIND_GETIP (added in #846).
To prevent that from happening again, also added Bazel tests that confirm stacktrace are still working.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[marekcirkos]

@drigz are you windows guy by any chance?

[sergiud]

Are you sure these tests are necessary? There's a stacktrace_unittest.cc already. Either way, the naming scheme does not follow glog conventions.

[marekcirkos]

Thanks @sergiud I missed existing tests. I added Bazel rules for them, skipped few however, as I didn't have time to investigate why they are broken.

There is some caveat, though. Current symbolize_test and stacktrace_test will miss this issue anyway. Mainly because they will just "skip" due to missing HAVE_SYMBOLIZE or/and HAVE_STACKTRACE definitions eg. https://github.com/google/glog/blob/master/src/stacktrace_unittest.cc#L236.

From what I can see they are defined in here https://github.com/google/glog/blob/master/src/utilities.h#L104.

We could define those flags in Bazel, but that might fail tests in legit platforms that do not support them.

[drigz]

@drigz are you windows guy by any chance?

I'm afraid not, almost all my Windows development experience comes from trying to keep this build working on Windows... Why do you ask?

If it's about the current error:

c:\b\k4x6jequ\execroot\__main__\src\utilities.h(108): error C2220: warning treated as error - no 'object' file generated
--
  | c:\b\k4x6jequ\execroot\__main__\src\utilities.h(108): warning C4005: 'HAVE_STACKTRACE': macro redefinition
  | src/stacktrace_unittest.cc: note: see previous definition of 'HAVE_STACKTRACE'

I hope that /IGNORE:C4005 could help (IGNORE, C4005).

However, if this is too fiddly, we could also consider a different approach: a bazel-only test that runs EXPECT_TRUE(HAVE_STACKTRACE). I guess that the bazel tests only run on CI, and we do expect stacktrace support on Linux/macOS/Windows - so hopefully we don't need to worry about platforms that truly don't support stacktraces.

That said, I'd accept this PR even without tests, as adding a new test just to check we've kept copts up to date feels like scope creep.

Thanks again for taking the time to get to the bottom of this!

[marekcirkos]

@drigz adjusted test per your suggestion :)

[codecov-commenter]

Codecov Report

Merging #851 (8eea553) into master (acc60d0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #851   +/-   ##
=======================================
  Coverage   73.28%   73.28%           
=======================================
  Files          17       17           
  Lines        3290     3290           
=======================================
  Hits         2411     2411           
  Misses        879      879           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[drigz]

Bazel changes LGTM, @sergiud if you're happy with the change to stacktrace_unittest.cc please submit.

[sergiud]

Thanks!