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 section on how to build debug build #22510

Closed
wants to merge 7 commits into from
Closed

Conversation

tlbdk
Copy link
Contributor

@tlbdk tlbdk commented Aug 24, 2018

There is very little documentation on how to build a debug build and a quick google is less than helpful, this makes generating useful core dumps for reported issues harder than it should be.

The idea is that the nice people that help debug issue could have something to point for the common folks that were unlucky enough to hit a segmentation fault but pretty much have been happy using the official binaries and don't know too much about how the build or debug process works.

For extra points, it would be nice if there were an official debug build that one could just download.

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Aug 24, 2018
@tlbdk
Copy link
Contributor Author

tlbdk commented Aug 24, 2018

@addaleax Would this work?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Maybe also mention that a core dump is much more useful when the corresponding original binary is also available?

BUILDING.md Outdated
@@ -253,6 +253,23 @@ To install this version of Node.js into a system directory:
$ [sudo] make install
```

#### Building a debug build

If you run into a issue where Node.js segfaults and the "segfault-handler" does not provide enough information, you might have to build a debug build of Node.js to get enough information to debug the issue further, this is done by :
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We generally wrap lines at 80 characters (and there’s an extra space before the :)

This can also be taken care of when landing, I’d say.

Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

LGTM with a few nits. Good work.

BUILDING.md Outdated
@@ -253,6 +253,23 @@ To install this version of Node.js into a system directory:
$ [sudo] make install
```

#### Building a debug build

If you run into a issue where Node.js segfaults and the "segfault-handler" does not provide enough information, you might have to build a debug build of Node.js to get enough information to debug the issue further, this is done by :
Copy link
Member

Choose a reason for hiding this comment

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

Please replace 'a issue' -> 'an issue'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can I suggest:

If you run into a issue where Node.js does not provide enough debug information, you can try to build a debug enabled binary:

Copy link
Member

Choose a reason for hiding this comment

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

@refack I’d prefer something that makes clear that this relates almost exclusively to native errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you run into a issue where the information provided by the JS stack trace is not enough, or if you suspect the error happens outside of the JS VM, you can try to build a debug enabled binary.

BUILDING.md Outdated
$ make -j4
```

A make with "./configure --debug" generates two binaries, the regular release one in "out/Release/node" and a debug build in "out/Debug/node", only the release version is actually installed when you run "make install".
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure but I think this 'A' is not needed here, so it's just 'Make with ...' or 'make with'.

Also, I think it'll be better with 'a debug build in' -> 'a debug binary in'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also maybe it is worth to replace double quotes with backticks for all 4 cases here.

BUILDING.md Outdated

```console
$ ./configure --debug
$ make -j4
Copy link
Contributor

Choose a reason for hiding this comment

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

I might suggest make -C out BUILDTYPE=Debug -j4 to avoid unnececerily building the Release binary. That could allow you to replace the following paragraph with:

If the build finished successfully, the debug enabled binary will be at `out/Debug/node`

@refack
Copy link
Contributor

refack commented Aug 24, 2018

Hello @tlbdk and thank you for the contribution 🥇

@refack
Copy link
Contributor

refack commented Aug 24, 2018

FYI @tlbdk you can check that the email that is configured in your local git is to you preference (can bee see here). We use this only to properly attributing your changes to you in the AUTHORS file and the changelog.
If you would like for the Github UI to link the commit to your account and award you the Contributor label after the changes have been merged, make sure this local email is also added to your GitHub email list.

BUILDING.md Outdated

A make with "./configure --debug" generates two binaries, the regular release one in "out/Release/node" and a debug build in "out/Debug/node", only the release version is actually installed when you run "make install".

To use the debug build with with all the normal dependencies overwrite the release version in the install directory:
Copy link
Contributor

Choose a reason for hiding this comment

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

with with -> with

@tlbdk
Copy link
Contributor Author

tlbdk commented Aug 29, 2018

We implemented all the requested changes and added a bit about the issue you can run into with gdb not matching the platform the core dump was captured on, I think it is ready to merge. Do we need to do anything more?

Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

Just a few language/phrasing nits.

BUILDING.md Outdated

`make` with `./configure --debug` generates two binaries, the regular release
one in `out/Release/node` and a debug binary in `out/Debug/node`, only the
release version is actually installed when you run `make install`.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: unnecessary space at the beginning.

BUILDING.md Outdated
release version is actually installed when you run `make install`.

To use the debug build with all the normal dependencies overwrite the release
version in the install directory:
Copy link
Member

Choose a reason for hiding this comment

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

Also space.

BUILDING.md Outdated
@@ -280,6 +280,17 @@ When using the debug binary, core dumps will be generated in case of crashes.
These core dumps are useful for debugging when provided with the
corresponding original debug binary and system information.

Reading the core dump requires a gdb that was build on the same platform as the
Copy link
Member

@lundibundi lundibundi Aug 29, 2018

Choose a reason for hiding this comment

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

I'd suggest:
Reading the core dump requires gdb built on the same platform core dump was captured on, wdyt?

BUILDING.md Outdated
@@ -280,6 +280,17 @@ When using the debug binary, core dumps will be generated in case of crashes.
These core dumps are useful for debugging when provided with the
corresponding original debug binary and system information.

Reading the core dump requires a gdb that was build on the same platform as the
core dump was captured, so 64bit gdb if node was build as 64bit and Linux gdb if node
Copy link
Member

Choose a reason for hiding this comment

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

so 64bit gdb if node was build as 64bit and Linux gdb if node... -> (i.e. 64bit gdb for node built on a 64bit system, Linux gdb for node built on Linux) wdyt?

BUILDING.md Outdated
@@ -280,6 +280,17 @@ When using the debug binary, core dumps will be generated in case of crashes.
These core dumps are useful for debugging when provided with the
corresponding original debug binary and system information.

Reading the core dump requires a gdb that was build on the same platform as the
core dump was captured, so 64bit gdb if node was build as 64bit and Linux gdb if node
was built on Linux else you might get errors like "not in executable format: File format not recognized".
Copy link
Member

Choose a reason for hiding this comment

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

s/else/otherwise/

Also, shouldn't might be will, as I think it cannot work at all for such cases?

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Some nits.

BUILDING.md Outdated
These core dumps are useful for debugging when provided with the
corresponding original debug binary and system information.

Reading the core dump requires gdb built on the same platform core dump was
Copy link
Contributor

Choose a reason for hiding this comment

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

gdb -> `gdb` here and below?

BUILDING.md Outdated
corresponding original debug binary and system information.

Reading the core dump requires gdb built on the same platform core dump was
captured on(i.e. 64bit gdb for node built on a 64bit system, Linux gdb
Copy link
Contributor

Choose a reason for hiding this comment

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

on(i.e. -> on (i.e. (missing space)

Copy link
Contributor

Choose a reason for hiding this comment

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

node -> Node.js here and below (or node -> `node`).

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure but maybe 64bit -> 64 bit for consistency with other cases in this doc.

@lundibundi
Copy link
Member

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 30, 2018
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Doc format LGTM, but I am not sure if merge commits will make any trouble for the lander.

@addaleax
Copy link
Member

Landed in 030ef35, thanks for the PR! 🎉

@addaleax addaleax closed this Aug 31, 2018
addaleax pushed a commit that referenced this pull request Aug 31, 2018
PR-URL: #22510
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 31, 2018
PR-URL: #22510
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
PR-URL: #22510
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
PR-URL: #22510
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants