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

Add crash handling system #1157

Merged
merged 121 commits into from Apr 8, 2019

Conversation

Projects
None yet
6 participants
@optizone
Copy link
Contributor

optizone commented Feb 3, 2019

Work in progress. Tested only on Linux. Create this PR to be sure you dont mind that way of implementation.

closes #1002

Some screenshots and how it works:

First goes information about error (SIGSEGV, SIGILL, SIGABRT, etc).
image

Then user can choose directory to save dump (in case he decided to save it).
image

Then he can open issue on github by clicking href.
image

@ITAYC0HEN ITAYC0HEN changed the title Crash handling WIP: Crash handling Feb 3, 2019

@ITAYC0HEN

This comment has been minimized.

Copy link
Member

ITAYC0HEN commented Feb 3, 2019

Thank you! We will review this in the following days. Please let us know when it is ready, since you mentioned it is a WIP.

Also. is it tested on MAC & Linux & Windows?

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 3, 2019

Tested only on Linux. I will implement it on other OS after review and approval.

Show resolved Hide resolved src/common/CrashHandler.cpp Outdated
Show resolved Hide resolved src/common/CrashHandler.cpp Outdated
Show resolved Hide resolved src/common/CrashHandler.cpp Outdated
Show resolved Hide resolved src/common/CrashHandler.cpp Outdated
Show resolved Hide resolved src/common/CrashHandler.cpp Outdated
Show resolved Hide resolved src/common/CrashHandler.cpp Outdated
Show resolved Hide resolved src/common/CrashHandler.cpp Outdated
Show resolved Hide resolved src/common/CrashHandler.h
Show resolved Hide resolved src/common/CrashHandler.cpp Outdated
Show resolved Hide resolved src/common/CrashHandler.cpp Outdated
@thestr4ng3r
Copy link
Member

thestr4ng3r left a comment

CMake and Meson implementations are still missing.

Show resolved Hide resolved src/Cutter.pro Outdated
@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 3, 2019

Actually, I cant understand why travis is not passing. It says that there is no "third_party/lss/linux_syscall_support.h", when there is one. Maybe, it is because lss is submodule or something. It would be great if you help me to figure this out.

@XVilka

This comment has been minimized.

Copy link
Member

XVilka commented Feb 5, 2019

We can consider also this https://github.com/chromium/crashpad

@optizone optizone force-pushed the optizone:signals branch from 5cb9290 to 9e5385b Feb 5, 2019

Show resolved Hide resolved src/common/CrashHandler.cpp Outdated

@optizone optizone force-pushed the optizone:signals branch from 9e5385b to 17f4688 Feb 5, 2019

@thestr4ng3r
Copy link
Member

thestr4ng3r left a comment

Ok, so we discussed and came to the conclusion that we don't want to have breakpad as a submodule in the repository. Instead, it should be built manually in the CI and just (statically) linked against in the build systems. For the CI, you can write scripts to build it and add it to the /scripts directory.

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 5, 2019

Ok, so we discussed and came to the conclusion that we don't want to have breakpad as a submodule in the repository. Instead, it should be built manually in the CI and just (statically) linked against in the build systems. For the CI, you can write scripts to build it and add it to the /scripts directory.

Honestly, I cant understand what I should do and how it will look like at the end.

@xarkes

This comment has been minimized.

Copy link
Member

xarkes commented Feb 5, 2019

As he mentionned, you should come up with a .bat and a .sh that builds Cutter with breakpad, but doesn't use submodules.
So the script would do something like git clone; qmake ENABLE_BREAKPAD=True (in the idea, I don't remember the exact syntax).

@thestr4ng3r

This comment has been minimized.

Copy link
Member

thestr4ng3r commented Feb 5, 2019

Take python as the example. We don't have it as a submodule, but still build it ourselves:

Breakpad also has .pc files in its source, so I think it will work very similar here.

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 5, 2019

ok, I see

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 6, 2019

I know I should edit buid.bat and build.sh too.
Now I want to know that whether I've done that you asked.

@thestr4ng3r

This comment has been minimized.

Copy link
Member

thestr4ng3r commented Feb 6, 2019

Yep, it's a step in the right direction. For prepare_breakpad.py I don't think you have to use python, simple bash/bat scripts would be ok too.

But more importantly, check if you can use pkg-config instead of specifying the lib manually. If you specify a prefix to ./configure and after building run make install, you get a complete prefix with the .pc files that you can use from qmake. I think this should work quite smoothly. Again, check python for reference.
Then you can also put that into the .pro directly without using a .pri file.

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 6, 2019

Yeah, but it's more crossplatform I'd say, so why not?

Yep, it's a step in the right direction. For prepare_breakpad.py I don't think you have to use python, simple bash/bat scripts would be ok too.

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 6, 2019

But more importantly, check if you can use pkg-config instead of specifying the lib manually. If you specify a prefix to ./configure and after building run make install, you get a complete prefix with the .pc files that you can use from qmake. I think this should work quite smoothly. Again, check python for reference.
Then you can also put that into the .pro directly without using a .pri file.

There is a problem. .pc files are placed at /usr/local/lib/pkgconfig after make install, when pkg-config is searching at /usr/lib/pkgconfig.
Same thing with .a files.

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 6, 2019

And again. Why is some builds not passing?

@a1ext

This comment has been minimized.

Copy link
Member

a1ext commented Feb 6, 2019

And again. Why is some builds not passing?

(ClCompile target) ->
..\src\common\CrashHandler.cpp(18): fatal error C1083: Cannot open include file: 'client/windows/handler/exception_handler.h': No such file or directory
[C:\projects\cutter\build_x86\cutter.vcxproj]

Because it cannot find exception_handler.h. Probably it was removed with some submodule

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 7, 2019

Same thing when it was a submodule. And in this case every build would not pass, but there are some passed ones.

@optizone optizone force-pushed the optizone:signals branch from 9453df3 to ad66245 Feb 7, 2019

@a1ext

This comment has been minimized.

Copy link
Member

a1ext commented Feb 7, 2019

And again. Why is some builds not passing?

You can see the error if you click on Details -> click on red job -> go down through log to, error will be there

Show resolved Hide resolved scripts/prepare_breakpad.py Outdated
Show resolved Hide resolved src/Cutter.pro Outdated
@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Feb 7, 2019

@thestr4ng3r
I did a lot of research and there are my thoughts.
If you make install into some directory, you can use PKG_CONFIG_PATH env. variable to use pkg-config. And you can set this variable in .yml files, so CI will build it. But you can't change it through qmake (in .pro file). So apparently you can't build Cutter if you didn't install breakpad into /usr.

@thestr4ng3r

This comment has been minimized.

Copy link
Member

thestr4ng3r commented Feb 7, 2019

Right, you can't set PKG_CONFIG_PATH inside the pro file. And you shouldn't. It's the user's (or CI's) responsibility to install breakpad somewhere and make sure PKG_CONFIG_PATH is set up correctly.

Show resolved Hide resolved src/Cutter.pro Outdated
Show resolved Hide resolved scripts/prepare_breakpad.sh Outdated

@optizone optizone force-pushed the optizone:signals branch from 907a8ad to a8a5c04 Mar 31, 2019

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Mar 31, 2019

@ITAYC0HEN

This comment has been minimized.

Copy link
Member

ITAYC0HEN commented Apr 1, 2019

I accepted this pr. @xarkes please finish the job and work with @optizone to fix the issues you find and want to solve

@xarkes

This comment has been minimized.

Copy link
Member

xarkes commented Apr 1, 2019

Will find some time tomorrow, I'm very busy these days.

@xarkes
Copy link
Member

xarkes left a comment

I think it looks good.

Show resolved Hide resolved docs/source/building.rst Outdated
@ITAYC0HEN

This comment has been minimized.

Copy link
Member

ITAYC0HEN commented Apr 3, 2019

@thestr4ng3r may we have you for a final review before merging this?

optizone added some commits Apr 3, 2019

@optizone

This comment has been minimized.

Copy link
Contributor Author

optizone commented Apr 6, 2019

@thestr4ng3r thestr4ng3r changed the base branch from master to breakpad Apr 8, 2019

@thestr4ng3r thestr4ng3r merged commit cb56573 into radareorg:breakpad Apr 8, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@optizone optizone deleted the optizone:signals branch Apr 8, 2019

thestr4ng3r added a commit that referenced this pull request Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.