Skip to content

doc: add unittest core dump instructions #22226

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.

Sign up for GitHub

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

Jump to bottom
Closed
jamesob wants to merge 1 commit into from
Closed

doc: add unittest core dump instructions #22226

jamesob wants to merge 1 commit into from
+26 −0

Conversation

@jamesob
Copy link
Contributor

@jamesob jamesob commented Jun 11, 2021

Quick note on how to get core dumps out of the unittests.

practicalswift
practicalswift reviewed Jun 11, 2021
View reviewed changes
Copy link
Contributor

@practicalswift practicalswift left a comment

Choose a reason for hiding this comment

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

Concept ACK

@jamesob jamesob force-pushed the 2021-06-tests-core-dump branch from 09cea44 to bbeec50 Compare June 11, 2021 19:00
theStack
theStack reviewed Jun 11, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Concept ACK

Possible follow-ups:

  • move 2nd and 3rd paragraph into more general documentation section; IMHO knowing how to find causes for crashes is useful not only for the unit-tests, but also for all other binaries that potentially segfault
  • also describe how to analyze core-dumps via lldb (if clang is used rather than gcc)

@jamesob
Copy link
Contributor Author

jamesob commented Jun 12, 2021

describe how to analyze core-dumps via lldb (if clang is used rather than gcc)

FWIW gdb analyzes clang-built core dumps just fine for me, but maybe you meant something different.

theStack
theStack approved these changes Jun 12, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

describe how to analyze core-dumps via lldb (if clang is used rather than gcc)

FWIW gdb analyzes clang-built core dumps just fine for me, but maybe you meant something different.

Oh, it seems I had an ancient gdb version installed on my system, that didn't support the right DWARF version (leading me to the wrong conclusion that for clang-built core dump analysis only lldb works -- d'oh!).

ACK bbeec50921c2cfcf054ba843bda392d310de5782 📜

@jamesob
Copy link
Contributor Author

jamesob commented Jun 21, 2021

Okay for merge?

jonatack
jonatack reviewed Jun 21, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Thanks for adding this. Testing the instructions.

src/test/README.md Outdated
Copy link
Member

@jonatack jonatack Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

"Boost" is capitalized elsewhere in this file.

Suggested change
By default, the boost test runner will intercept system errors and not produce a core
By default, the Boost test runner will intercept system errors and not produce a core

jonatack
jonatack reviewed Jun 21, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK with two suggestions (one above, one below)

@maflcko
Copy link
Member

maflcko commented Jun 22, 2021

review ACK bbeec50921c2cfcf054ba843bda392d310de5782

Personally I just use valgrind which will print the stack when the dump is created

@jamesob
Copy link
Contributor Author

jamesob commented Jul 8, 2021

Ready for merge? Or I can add a note about valgrind?

maflcko
maflcko reviewed Jul 8, 2021
View reviewed changes
@jamesob jamesob force-pushed the 2021-06-tests-core-dump branch from bbeec50 to 3ad9b71 Compare July 20, 2021 14:25
@jamesob
Copy link
Contributor Author

jamesob commented Jul 20, 2021

Incorporated feedback from @MarcoFalke and @jonatack.

@practicalswift
Copy link
Contributor

practicalswift commented Jul 24, 2021

cr ACK 3ad9b71f48d2783b534264f53e059db4f239ea55

jonatack
jonatack reviewed Jul 28, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

This seems like a helpful improvement.

re-ACK modulo suggestions

src/test/README.md Outdated
Copy link
Member

@jonatack jonatack Jul 28, 2021

Choose a reason for hiding this comment

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

Suggested change
that as well. By default, the boost test runner will intercept system errors and not
that as well. By default, the Boost test runner will intercept system errors and not

src/test/README.md Outdated
Copy link
Member

@jonatack jonatack Jul 28, 2021

Choose a reason for hiding this comment

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

linter appeasement

Suggested change
(on Linux platforms, the file name will likely depend on the contents of
(on Linux platforms, the file name will likely depend on the contents of

src/test/README.md Outdated
Copy link
Member

@jonatack jonatack Jul 28, 2021

Choose a reason for hiding this comment

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

Suggested change
gdb src/test/test_bitcoin core
gdb ./src/test/test_bitcoin core

Copy link
Member

@jonatack jonatack Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(you write gdb ./src/test/test_bitcoin along with bt at the top of the diff)

theStack
theStack reviewed Jul 30, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

almost re-ACK (linter is not happy yet)

Also I agree that the other suggestions by jonatack above (#22226 (comment), #22226 (comment)) should be tackled.

@laanwj
Copy link
Member

laanwj commented Sep 16, 2021

LGTM, but yes it's failing the linters.

@jamesob
doc: test: unittest segfault gdb
1231338 
Feedback from Jon Atack and Marco Falke.
@jamesob jamesob force-pushed the 2021-06-tests-core-dump branch from 3ad9b71 to 1231338 Compare September 16, 2021 22:03
@jamesob
Copy link
Contributor Author

jamesob commented Sep 16, 2021

Fixed, thanks for the looks.

theStack
theStack approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK 1231338

maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Sep 17, 2021
Merge bitcoin/bitcoin#22226: doc: add unittest core dump instructions
6401de0 
1231338 doc: test: unittest segfault gdb (James O'Beirne)

Pull request description:

  Quick note on how to get core dumps out of the unittests.

ACKs for top commit:
  theStack:
    ACK 1231338

Tree-SHA512: d749d9117f96af85f9053884c57df766ac1d29e57b2555d4fc63bd9dc29df47487954cee1c7cd78ee420ae1c9c7da7ddc9797b6c636ce7641eae20622eaa3fee
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 17, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@fanquake
Copy link
Member

fanquake commented Sep 17, 2021

This has been merged, but wasn't closed by GitHub.

@fanquake fanquake closed this Sep 17, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2021
Merge bitcoin#22226: doc: add unittest core dump instructions
d0995ab 
1231338 doc: test: unittest segfault gdb (James O'Beirne)

Pull request description:

  Quick note on how to get core dumps out of the unittests.

ACKs for top commit:
  theStack:
    ACK 1231338

Tree-SHA512: d749d9117f96af85f9053884c57df766ac1d29e57b2555d4fc63bd9dc29df47487954cee1c7cd78ee420ae1c9c7da7ddc9797b6c636ce7641eae20622eaa3fee
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@jonatack jonatack jonatack left review comments

@maflcko maflcko maflcko left review comments

@practicalswift practicalswift practicalswift left review comments

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Docs Needs rebase Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jamesob @maflcko @practicalswift @laanwj @DrahtBot @fanquake @theStack @jonatack