Page MenuHome

Windows: Add stacktrace for release builds
ClosedPublic

Authored by Ray Molenkamp (LazyDodo) on Apr 24 2020, 10:59 PM.

Details

Summary

Feature can be toggled by the WITH_WINDOWS_PDB cmake option,
by default it will bundle a stripped PDB which is significantly
smaller, however the full symbols can be shipped by settings
WITH_WINDOWS_STRIPPED_PDB to Off

Merge remote-tracking branch 'origin/master' into tmp_crashlog

Diff Detail

Repository
rB Blender
Branch
tmp_crashlog (branched from master)
Build Status
Buildable 7729
Build 7729: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Cleanup pass & fill in missing bits of information.
  • Merge remote-tracking branch 'origin/master' into tmp_crashlog
  • clang-format
  • move required libs to platform/platform_win32.cmake
  • Merge remote-tracking branch 'origin/master' into tmp_crashlog
  • last cleanup pass
  • limit stacktrace to a 16 line window on the tracker.

Think the technical bits are ready for review, the html based submission wizard may need "some tweaks" though, perhaps have an actual webguy look at it (i'll be honest, 90% of the code/css is cut and pasted from w3schools in 20 minutes) adding @Dalai Felinto (dfelinto) as a reviewer to guide that into a direction we want.

  • skip # in the blender version header when reporting to the tracker.
  • Make the reporting UI optional so we don't get reports from branches and forks we have no control over.

Can there be a diff for just the part that makes it so we get a strack trace in the terminal or debug logs? That could be approved quickly.

And then a new one for the dialog and HTML based wizard?

I would like to have a wizard like this. But ideally it would be part of the bug submission form on developer.blender.org instead of embedded in Blender. So that we can keep updating it and don't need to add a bunch of custom CSS.

Yes the submission bit is already behind a cmake switch, we could easily take that to a different diff.

that being said, the change is big enough to be uncomfortable to be landing it in 2.83 still, 2.90 seems like the right place for this.

Sergey Sharybin (sergey) added inline comments.
source/blender/blenlib/intern/system.c
146–151

Is this related to this change or can this be committed separately with own explanation?

In general I'm not so convinced that submitting a back-trace to the tracker is all that useful.

There is some risk users will submit this thinking they have reported an issue, when we really need ways to redo the bug.


Having said that, the code-changes seem OK, but would rather backtrace support be split out from the HTML submission stuff (as @Brecht Van Lommel (brecht) mentions).

In general I'm not so convinced that submitting a back-trace to the tracker is all that useful.

Getting the user to report basic information like blender version or system_info.txt on the tracker at times is a struggle, anything we can automate in this department imho is an improvement for the people triaging the tracker.

There is some risk users will submit this thinking they have reported an issue, when we really need ways to redo the bug.

True but we could deal with that in the html wizard, while the tracker has a big textbox people mostly ignore, we can actively stop the wizard from progressing unless they give an answer to certain questions (as the wizard currently does for the repro steps)

However given this functionality is about to be axed, lets put a pin in this conversation and continue when this gets its own review.

source/blender/blenlib/intern/system.c
146–151

I had superfluous spacing at the beginning of the string which looked weird on the tracker report, so in that regard it was part of this diff. I see no issues moving this to it's own commit.

source/blender/blenlib/intern/system.c
146–151

Think you can just commit this part (unless you already did it).

  • Merge remote-tracking branch 'origin/master' into tmp_crashlog
  • remove reporting functionality
  • Add source line information in the trace if available
Brecht Van Lommel (brecht) requested changes to this revision.Apr 29 2020, 8:07 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenlib/intern/system_win32.c
33

Add empty line after this.

45–46

Leave for other patch.

48

It would define this as const int max_symbol_length = 100; in the relevant function instead of making it global.

52

ExceptionDescription -> windows_exception_description

100

Use lowercase bli_ or no prefix for static functions.

201

Define char module[MAX_PATH]; right above this line so its scope is clear.

284

Move this definition right above te32.dwSize = sizeof(THREADENTRY32);

286

Simplify to

HANDLE hThreadSnap = CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0);
source/creator/creator_signals.c
74

Can the global be defined in blenlib and declared here? The dependency should generally go in the other direction, and it's not clear to me that the current code works for executables that link in blenlib but don't include creator.

Alternatively, system_win32.c could define a void BLI_system_windows_exception_handler(EXCEPTION_POINTERS *ExceptionInfo); with most of the code from windows_exception_handler, and make it a static variable rather than a global one.

210–212

If this code moves into system_win32.c it could use ExceptionDescription().

This revision now requires changes to proceed.Apr 29 2020, 8:07 PM
Ray Molenkamp (LazyDodo) marked 9 inline comments as done.
  • Update with feedback from code-review.
source/blender/blenlib/intern/system_win32.c
52

to keep style with other functions bli_windows_get_exception_description

source/creator/creator_signals.c
210–212

I de-duplicated this code, however this code is kinda wacky, the comment claims

/* If this is a stack overflow then we can't walk the stack, so just show
 * where the error happened */

then proceeds to print the address for every exception but a stackoverflow....

i added a stackoverflow (2 in the debug menu, will be removed before landing) to test and the implementation i have now seems to work.

LGTM assuming debug code gets removed and comment addressed.

source/creator/creator_signals.c
74

This can be removed now?

This revision is now accepted and ready to land.Apr 29 2020, 10:00 PM
  • remove left over global.
  • revert test crashes.

some issues popped up during final testing when building with ninja, while i sort this out, when landing can i change the default for WITH_WINDOWS_PDB to On ?

I guess we would enable this by default for all builds except releases? I think that is reasonable.

Then we'd need something like this:

diff --git a/build_files/buildbot/slave_compile.py b/build_files/buildbot/slave_compile.py
index 65cadea..ddb059e1 100644
--- a/build_files/buildbot/slave_compile.py
+++ b/build_files/buildbot/slave_compile.py
@@ -37,6 +37,10 @@ def get_cmake_options(builder):
     elif builder.platform == 'win':
         options.extend(['-G', 'Visual Studio 15 2017 Win64'])
         options.extend(['-DPOSTINSTALL_SCRIPT:PATH=' + post_install_script])
+
+        info = buildbot_utils.VersionInfo(builder)
+        if info.version_cycle == 'release':
+            options.append('-DWITH_WINDOWS_PDB=OFF')
     elif builder.platform == 'linux':
         config_file = "build_files/buildbot/config/blender_linux.cmake"

We could consider enabling it for official releases as well, I'm not sure.

  • Merge remote-tracking branch 'origin/master' into tmp_crashlog
  • Fix building with ninja and/or clang
  • build bot changes

The note I had about the pdb file name being problematic but cpack doing
the right thing, should have been a warning something was very very wrong.

ninja used a different path for the pdb and the whole thing fell apart.

It's now been properly fixed and

Build flags have been changed to:

Full pdb (always)         : blender_private.pdb located in the creator build folder
stripped pdb (if enabled) : blender_public.pdb located in the creator build folder

if WITH_WINDOWS_PDB is On, cmake will install the correct pdb next to the blender binary
naming it 'blender.pdb'

A symbol loader has been added to look for this alternative pdb file name, however if
it is detected the private symbols are already loaded (generally the case on a developer
work station), no attemped is made to load the stripped symbol file.

The significant new code in bli_private_symbols_loaded and bli_load_symbols and
wouldn't mind one last review on those.

clang does currently not seem support the stripped pdb link option, cmake will warn
about this and revert to using the full pdb.

Brecht Van Lommel (brecht) requested changes to this revision.Apr 30 2020, 7:32 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenlib/intern/system_win32.c
310

Comments: capitalize and end sentence in period.

311–312

Use {}

344

I think this should just silently do nothing instead of printing an error?

source/creator/CMakeLists.txt
1103

Remove debug print

This revision now requires changes to proceed.Apr 30 2020, 7:32 PM
  • update with feedback
Ray Molenkamp (LazyDodo) marked 2 inline comments as done.Apr 30 2020, 7:36 PM
Ray Molenkamp (LazyDodo) marked an inline comment as done.

Still need to remove that debug print.

This revision is now accepted and ready to land.Apr 30 2020, 7:54 PM
Traceback (most recent call last):
  File "../blender.git/build_files/buildbot/slave_compile.py", line 119, in <module>
  File "../blender.git/build_files/buildbot/slave_compile.py", line 89, in cmake_configure
  File "../blender.git/build_files/buildbot/slave_compile.py", line 41, in get_cmake_options
  File "C:\b\win64_cmake_vs2017\win64_cmake_vs2017\blender.git\build_files\buildbot\buildbot_utils.py", line 85, in __init__
    self.hash = self._parse_header_file(buildinfo_h, 'BUILD_HASH')[1:-1]
  File "C:\b\win64_cmake_vs2017\win64_cmake_vs2017\blender.git\build_files\buildbot\buildbot_utils.py", line 107, in _parse_header_file
    with open(filename, "r") as file:
FileNotFoundError: [Errno 2] No such file or directory: 'C:\\b\\win64_cmake_vs2017\\win64_cmake_vs2017\\build\\win64_cmake_vs2017\\source\\creator\\buildinfo.h'

the buildbot did not enjoy this, not obvious at first sight why the build path has win64_cmake_vs2017 in there 3 times, i reverted this change for now.

@Brecht Van Lommel (brecht) / @Sergey Sharybin (sergey) either of you willing to take a look ? (or if we go, we'll keep it for release that's fine with me)

Ray Molenkamp (LazyDodo) reopened this revision.EditedApr 30 2020, 10:01 PM

reverted due to older cmake issues, urgh.. there's no end to this thing..

This revision is now accepted and ready to land.Apr 30 2020, 10:01 PM
  • deal with older cmake better, tested on the same version as the buildbot (3.12)

Allright got that sorted out, I'll have another go at landing this tomorrow, end of the day here, and i want to be available in case all hell breaks lose again.

while this is stalled: what's the plan for release builds? personally i'd say we see how well this functions in the daily, and push the problem forward towards the release of 2.90 to decide if we ship with or without it.

Ok, we can postpone the decision of including this in releases, and just always enable it for now.

  • remove buildbot changes
  • Merge remote-tracking branch 'origin/master' into tmp_crashlog
  • revert white-space changes.