Page MenuHomePhabricator

[Breakpoint] Make breakpoint language agnostic
ClosedPublic

Authored by xiaobai on May 9 2019, 11:25 AM.

Details

Summary

Breakpoint shouldn't need to depend on any specific details from a
programming language. Currently the only language-specific detail it takes
advantage of are the different qualified names an objective-c method name might
have when adding a name lookup. This is reasonably generalizable.

The current method name I introduced is "GetVariantMethodNames", which I'm not
particularly tied to. If you have a better suggestion, please do let me know.

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.May 9 2019, 11:25 AM
JDevlieghere added inline comments.May 9 2019, 11:31 AM
include/lldb/Target/Language.h
197 ↗(On Diff #198878)

GetMethodNameVariants?

source/Breakpoint/BreakpointResolverName.cpp
237 ↗(On Diff #198878)

You could write this as

if (Language *lang = Language::FindPlugin(m_language)) {
  add_variant_funcs(lang);
} else {
  // Most likely m_language is eLanguageTypeUnknown. We check each language for
  // possible variants or more qualified names and create lookups for those as
  // well.
  Language::ForEach(add_variant_funcs);
}
jingham accepted this revision.May 9 2019, 1:22 PM

That looks fine, thanks for untangling this.

I do like Jonas' suggestion better. GetVariantMethodNames sounds like "VariantMethods" are a specific thing and you are trying to find their names, whereas this is just variants of the method name. GetMethodNameVariants makes that clearer.

I was going to quibble a bit about "Method" since this can also happen for regular functions. This isn't entirely theoretical, macOS used to used to have different variants of some of the standard library routines based on the whether the program was running in strict posix mode or had xmm registers, etc. Then it had the kernel map in the little block of function pointers and used assembly tricks so the function name would call through this block. In that case since lldb didn't know which instance of the function was going to be used we had to look for these variants and break on all of them. That code is gone now - macOS switched to dynamic resolver functions instead. But this would have been another use for these "Variants".

OTOH, I can't think of a good name for "callable entities" that isn't awful, so I'm fine with Method...

This revision is now accepted and ready to land.May 9 2019, 1:22 PM

Thanks for the feedback!

I don't quite feel that "Method" is a great descriptor here either, but I think that it's probably fine for now until we can think of a better name. I'll update this when I get the chance.

xiaobai updated this revision to Diff 198911.May 9 2019, 2:30 PM

Rework function
GetVariantMethodNames -> GetMethodNameVariants

JDevlieghere accepted this revision.May 9 2019, 4:41 PM
labath added a subscriber: labath.May 9 2019, 11:19 PM
labath added inline comments.
include/lldb/Target/Language.h
198 ↗(On Diff #198911)

Return the vector by value ?

xiaobai updated this revision to Diff 199113.May 10 2019, 5:46 PM
  • Fix minor bug
  • Return vector by value instead of passing in one by parameter.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2019, 8:30 PM