Skip to content

[Demangler] Make Node::addChild(NULL, ...) always assert. #41452

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

Merged
merged 5 commits into from
Mar 25, 2022

Conversation

al45tair
Copy link
Contributor

We need this in order to gather relevant information when this situation arises; it's clearly happening (e.g. rdar://86071019), but not often enough for it to trigger in a build with assertions enabled.

rdar://89139049

We need this in order to gather relevant information when this situation
arises; it's clearly happening (e.g. rdar://86071019), but not often
enough for it to trigger in a build with assertions enabled.

rdar://89139049
@al45tair al45tair requested review from jckarter and mikeash February 18, 2022 12:17
@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

Copy link
Contributor

@mikeash mikeash left a comment

Choose a reason for hiding this comment

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

LGTM. Would be worth considering if there's any additional info to log here that might be helpful (like the parent's kind, child count, other contents?). Since the turnaround time on this is not going to be real quick.

@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

@al45tair
Copy link
Contributor Author

LGTM. Would be worth considering if there's any additional info to log here that might be helpful (like the parent's kind, child count, other contents?). Since the turnaround time on this is not going to be real quick.

Good plan. I've made the failAssert() function dump the node that failed the assertion, which should supply plenty of extra information.

@al45tair al45tair force-pushed the eng/PR-89139049 branch 2 times, most recently from b7bd245 to 8bccf9d Compare March 11, 2022 11:38
This should help to debug things when things go wrong.

rdar://89139049
@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

@al45tair al45tair changed the title Make Node::addChild(NULL, ...) always assert. [Demangler] Make Node::addChild(NULL, ...) always assert. Mar 12, 2022
I was trying to use fatalError() here, but that is in libswiftRuntime,
which isn't linked into everything that has the demangler code in it.
The upshot is that we'd have to touch a lot of other projects to use
fatalError().

rdar://89139049
@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

2 similar comments
@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

The demangling library can't use the error handling from the main runtime
because it isn't always linked with it.  However, it's useful to have
some error handling, and in particular to be able to get data into the
crash logs.

This is complicated because of the way the demangling library gets used,
the upshot of which is that I've had to add a second object library just
for libswiftCore's use, so that the demangler will use the runtime's
error handling functions when present, and fall back on its own when
they aren't.

rdar://89139049
@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

Crash reporter integration was only enabled for iOS.  Enable it for
any Darwin platform, but disable it for the minimal build.

Also fix up a couple of issues that popped up when it was enabled.

rdar://89139049
@al45tair
Copy link
Contributor Author

@swift-ci Please smoke test

@al45tair
Copy link
Contributor Author

@swift-ci Please test

al45tair added a commit to al45tair/swift that referenced this pull request Mar 25, 2022
Some of the functions inside the SWIFT_STDLIB_HAS_TYPE_PRINTING conditional
are now needed even in the minimal build as a result of swiftlang#41452.

rdar://90839754
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants