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.

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

Fixing up numba_do_raise #6017

Merged
merged 4 commits into from Aug 3, 2020
Merged

Fixing up numba_do_raise #6017

merged 4 commits into from Aug 3, 2020

Conversation

sklam
Copy link
Member

@sklam sklam commented Jul 21, 2020 •
edited

This patch attempts to fix an extra decref in numba_do_raise() as discovered in #5922 which will result in segfault as described in #5922 (comment)

The patch refactored the numba_do_raise() function into multiple functions for easier debugging. As a result, there are less incref/decref needed. Sub-functions are mostly borrowing references.

@sklam sklam mentioned this pull request Jul 21, 2020
Merged
@sklam sklam added this to the Numba 0.51 RC milestone Jul 22, 2020
@sklam
Copy link
Member Author

sklam commented Jul 22, 2020

#6024 tested this PR combined with #5922

@sklam sklam mentioned this pull request Jul 22, 2020
Closed
@sklam sklam marked this pull request as ready for review July 23, 2020 13:19
@stuartarchibald stuartarchibald self-assigned this Jul 23, 2020
stuartarchibald
stuartarchibald reviewed Jul 24, 2020
View reviewed changes
Copy link
Contributor

@stuartarchibald stuartarchibald 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 refactoring this, it's much easier to follow now. Think this just needs a few comments adding about what functions do/their args and this is good to go.

numba/_helperlib.c Outdated
/*
* exc cannot be NULL
* value can be NULL
* loc can be NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

Where's loc?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was refactored out. the line can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 07415eb

numba/_helperlib.c
return 1;
}

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to expand the comment to include information about what this function does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 07415eb

numba/_helperlib.c Outdated
Py_XDECREF(type);
return 0;
/*
* PyExceptionClass_Check(exc) must be True
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to expand the comment to include information about what this function does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 07415eb

numba/_helperlib.c
Py_XDECREF(type);
return 0;
static
int reraise_exc_is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to expand the comment to include information about what this function does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 07415eb

numba/_helperlib.c
PyObject *exc = NULL, *type = NULL, *value = NULL, *loc = NULL;

static
void traceback_add_loc(PyObject *loc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to include a comment about what this function does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 07415eb

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jul 24, 2020
@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jul 30, 2020
@sklam
Copy link
Member Author

sklam commented Jul 30, 2020

BFID: numba_smoketest_cpu_58

@sklam sklam added the Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm label Jul 30, 2020
@sklam
Copy link
Member Author

sklam commented Jul 31, 2020

BF passed except the aarch64 problem that is affecting other PRs.

stuartarchibald
stuartarchibald reviewed Aug 3, 2020
View reviewed changes
numba/_helperlib.c Outdated Show resolved Hide resolved
stuartarchibald
stuartarchibald reviewed Aug 3, 2020
View reviewed changes
numba/_helperlib.c Outdated Show resolved Hide resolved
stuartarchibald
stuartarchibald reviewed Aug 3, 2020
View reviewed changes
numba/_helperlib.c Outdated Show resolved Hide resolved
stuartarchibald
stuartarchibald reviewed Aug 3, 2020
View reviewed changes
numba/_helperlib.c Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald added BuildFarm Passed For PRs that have been through the buildfarm and passed and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm labels Aug 3, 2020
@stuartarchibald
Copy link
Contributor

BF passed except the aarch64 problem that is affecting other PRs.

Thanks, don't think there's anything aarch64 specific in here so will accept as a pass.

Thanks for 07415eb, I've pushed a few typo fixes in 19ce816. Assuming build passes this is good to go.

@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Aug 3, 2020
@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on CI Review etc done, waiting for CI to finish labels Aug 3, 2020
@stuartarchibald stuartarchibald merged commit 2e5169a into numba:master Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartarchibald stuartarchibald stuartarchibald left review comments

Assignees

@stuartarchibald stuartarchibald

Labels
5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed
Projects
None yet
Milestone
Numba 0.51 RC
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sklam @stuartarchibald