This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Fix parsing of $ld$ symbols with $ in them
AbandonedPublic

Authored by keith on Jun 16 2022, 6:08 PM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

Swift symbols have dollar signs in them, so splitting on this
incorrectly resulted in a bunch of symbols just with the name _ being
considered, and extra info ignored. Currently we don't do anything with
these symbol names besides check that they're non-empty, so this doesn't
impact that, but I believe we need to start handling these:

https://github.com/llvm/llvm-project/issues/56074

Diff Detail

Event Timeline

keith created this revision.Jun 16 2022, 6:08 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 16 2022, 6:08 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
keith requested review of this revision.Jun 16 2022, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 6:08 PM
int3 added a subscriber: int3.Jun 16 2022, 8:28 PM

Could we have a test?

keith added a comment.Jun 16 2022, 8:29 PM

I don't think we can until we're actually doing something in the case the symbol is non-empty, which will require some more changes

int3 added a comment.Jun 16 2022, 8:36 PM

Hm so is this actually fixing the handling of these symbols, or just raising an assertion error when we encounter them?

keith added a comment.Jun 16 2022, 8:38 PM

Just raising an assertion in the case we hit some that we don't expect. I started looking at actually handling them but I don't think the changes will be very trivial, so I just filed https://github.com/llvm/llvm-project/issues/56074 until I have time

thakis added a subscriber: thakis.Jun 16 2022, 8:42 PM

The rsplit() is correct. I don't think the assert adds a ton of value.

Not sure it makes sense to land this without the rest of previous + sym handling as it can't be tested before then.

I have half a patch for that locally, so maybe just wait a few days?

int3 added a comment.Jun 16 2022, 8:45 PM

@keith gotcha. I think the commit message / title is confusing since this isn't an actual fix

keith added a comment.Jun 16 2022, 8:47 PM

The rsplit() is correct. I don't think the assert adds a ton of value.

no strong pref, i just figure this format shouldn't ever really change so it might be safer this way to avoid future breakages.

Not sure it makes sense to land this without the rest of previous + sym handling as it can't be tested before then.

I have half a patch for that locally, so maybe just wait a few days?

yea sounds fine, i just wanted to upload this asap in case someone did want to look into that issue before i did so they wouldn't have to re-fix this bug

keith abandoned this revision.Jul 19 2022, 1:59 PM