Skip to content

Fix a couple references to nullptr #63666

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
Merged
merged 2 commits into from
Feb 16, 2023
Merged

Fix a couple references to nullptr #63666

merged 2 commits into from
Feb 16, 2023

Conversation

bnbarham
Copy link
Contributor

@bnbarham bnbarham commented Feb 15, 2023 •
edited
Loading

Swift had a couple places that took a reference to a nullptr, but then never used it. This wasn't crashing, but now that C++ stdlib assertions are enabled, it will now assert in unique_ptr::operator* (which checks if the underlying pointer is a nullptr).

@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham
Copy link
Contributor Author

I've been running the tests again locally and there's more of these. I'll add those in tomorrow.

@bnbarham
[Frontend] Remove reference to uninitialized ASTContext
8ce26c7 
Certain actions do not initialize `ASTContext`, eg.
`TypecheckModuleFromInterface`. But `performAction` was always grabbing
a `ASTContext&`. With `_GLIBCXX_ASSERTIONS` and
`_LIBCPP_ENABLE_ASSERTIONS` now enabled in an asserts build, this hits
an assert in `unique_ptr::operator*` that checks for a null deref.
@bnbarham
[Test] Only print completion diagnostics when an ASTContext is available
fd4f51a 
swift-ide-test was creating a `CompilerInstance` and then passing its
`ASTContext` along for printing diagnostics. But `CompilerInstance` only
has an `ASTContext` if it has been initialized, which it hasn't.

Change the print to only print diagnostics when an `ASTContext` is
actually available.
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham
Copy link
Contributor Author

bnbarham commented Feb 15, 2023 •
edited
Loading

These two along with #63612 should fix these asserts.

@bnbarham bnbarham changed the title [Frontend] Remove reference to uninitialized ASTContext Fix a couple references to nullptr Feb 15, 2023
etcwilde
etcwilde approved these changes Feb 15, 2023
View reviewed changes
Copy link
Member

@etcwilde etcwilde left a comment

Choose a reason for hiding this comment

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

taking a reference to a nullptr

The fact that that wasn't crashing is really something else.

Thanks, I hate it. LGTM.

@etcwilde
Copy link
Member

8.3.2/5:

A reference shall be initialized to refer to a valid object or function. [ Note: in particular, a null reference cannot exist in a well-defined program, because the only way to create such a reference would be to bind it to the “object” obtained by indirection through a null pointer, which causes undefined behavior. As described in 9.6, a reference cannot be bound directly to a bit-field.]

reference; https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf

I think getting an Asan/UBsan bot might be a good idea. It would probably be red 98.9% of the time, but then we would be able to look at it and say "hey, this UB right here is bad and we should feel bad, but also, that's probably where my bug is coming from."

@bnbarham
Copy link
Contributor Author

Well, we do have an asan bot. It's just not even getting past compilation right now - https://ci.swift.org/job/oss-swift-incremental-ASAN-RA-macos. ubsan would be cool to have as well.

@bnbarham bnbarham merged commit 959e5bb into swiftlang:main Feb 16, 2023
@bnbarham bnbarham deleted the fix-new-assert branch February 16, 2023 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd approved these changes

@etcwilde etcwilde etcwilde approved these changes

@eeckstein eeckstein Awaiting requested review from eeckstein

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bnbarham @etcwilde @compnerd