Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[DebugInfo][NFC] Move ObjC Selector name handling to lib DebugInfo

Authored by fdeazeve on Aug 28 2023, 5:52 AM.



The DWARFLinker library has code to identify ObjC selector names, which is used
by the debug linker to generate accelerator table entries. In the future, we
would like the DWARF verifier to also have access to such code, so that it can
identify these names when verifying accelerator tables (e.g. debug_names).

This patch follows the same intent of D155723, where we also moved code
generating simplified template names.

Since this is moving code around and changing the log, we also replace raw
pointer manipulation with the more expressive
StringRef::{drop_front,take_front,...} methods.

We also change a test so that it verifies its output, and that requires having
dsymutil not write to stdout.

Diff Detail

Event Timeline

fdeazeve created this revision.Aug 28 2023, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2023, 5:52 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
fdeazeve requested review of this revision.Aug 28 2023, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2023, 5:52 AM
fdeazeve updated this revision to Diff 553906.Aug 28 2023, 6:12 AM

Make a test run the verifier.

fdeazeve edited the summary of this revision. (Show Details)Aug 28 2023, 6:12 AM
fdeazeve added a reviewer: debug-info.
fdeazeve added a project: debug-info.
fdeazeve updated this revision to Diff 553919.Aug 28 2023, 7:07 AM
fdeazeve edited the summary of this revision. (Show Details)

Remove duplicate line

Couple of minor nitpicks, otherwise this LGTM.


Why plural?


Is the = part necessary or will it be default constructed as empty like llvm::Optional?


If there are any comments to explain the latter two members, that would be nice.


maybe that's for a later patch, but this seems to be a complicated way of saying
split(" ")

fdeazeve added inline comments.Aug 28 2023, 10:06 AM

Because there are 4 such names. But to be honest, I'm not sure I understand what an objective C selector is; maybe just "ObjectiveCNames" would be better?


It is constructed empty like an llvm::Optional, so this = std::nullopt is not needed, I'll remove it.


I'll do some research to figure out what they actually are and add some comments

fdeazeve updated this revision to Diff 556072.Sep 6 2023, 1:18 PM

Address review comments


Agreed. I will do a second patch after the base changes are good.

aprantl added inline comments.Sep 6 2023, 3:15 PM

An ObjC selector is basically the name of a method:

When you send a message (~ call a method) in ObjC, you pass the selctor to objc_msgSend and it will look up the function behind that selector and invoke it. So if it's all selector names, let's keep it that way; it would be the right terminology.

aprantl accepted this revision.Sep 6 2023, 3:15 PM

In any case this LGTM with the minor editorial comments above incorporated.

This revision is now accepted and ready to land.Sep 6 2023, 3:15 PM
fdeazeve updated this revision to Diff 556144.Sep 7 2023, 7:08 AM

Address review comments

fdeazeve added inline comments.Sep 7 2023, 7:11 AM

Thank you for the pointers!

This revision was landed with ongoing or failed builds.Sep 7 2023, 7:13 AM
This revision was automatically updated to reflect the committed changes.