Page MenuHomePhabricator

[Target] Generalize some behavior in Target::SymbolsDidLoad
ClosedPublic

Authored by xiaobai on Jun 2 2019, 1:33 PM.

Details

Summary

SymbolsDidLoad is currently only implemented for ObjCLanguageRuntime,
but that doesn't mean that it couldn't be useful for other Langauges. Although
this change seems like it's generalizing for the sake of purity, this removes
Target's dependency on ObjCLanguageRuntime.

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.Jun 2 2019, 1:33 PM
labath added inline comments.Jun 3 2019, 1:21 AM
source/Target/Target.cpp
1670–1672 ↗(On Diff #202620)

Same question about runtime iteration as in the other review.

clayborg requested changes to this revision.Jun 3 2019, 8:40 AM
clayborg added a subscriber: clayborg.

Seems like we need a Process::SymbolsDidLoad(...) function that we can call. It should iterate over all loaded language runtimes (only loaded ones) and forward the list.

source/Target/Target.cpp
1670 ↗(On Diff #202620)

Seems like we should add a Process::SymbolsDidLoad() method that matches this method's signature and just call that. Process would iterate across all loaded language runtimes and call SymbolsDidLoad on any?

This revision now requires changes to proceed.Jun 3 2019, 8:40 AM
xiaobai marked 2 inline comments as done.Jun 3 2019, 10:09 AM
xiaobai added inline comments.
source/Target/Target.cpp
1670 ↗(On Diff #202620)

Instead of adding Process::SymbolsDidLoad, I could instead iterate here over the loaded language runtimes with Process::GetLanguageRuntimes and call LanguageRuntime::SymbolsDidLoad on each one. What do you think? This would prevent us from having 3 classes with the method SymbolsDidLoad with the same signature.

1670–1672 ↗(On Diff #202620)

Right, we should definitely iterate over loaded language runtimes instead of just using the ObjC runtime. Will update.

clayborg added inline comments.Jun 3 2019, 10:13 AM
source/Target/Target.cpp
1670 ↗(On Diff #202620)

That is fine as long as we iterate over all loaded language runtimes I am good.

xiaobai updated this revision to Diff 202782.Jun 3 2019, 1:22 PM

Address comments

clayborg accepted this revision.Jun 3 2019, 1:26 PM
This revision is now accepted and ready to land.Jun 3 2019, 1:26 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 4:09 PM