Page MenuHomePhabricator

Add ability to override JIT expr compiler options.
ClosedPublic

Authored by ldrumm on Dec 15 2015, 7:12 AM.

Details

Summary

Runtimes should be able to pass custom compilation options to the JIT for their stack frame. This patch adds a custom expression options member class to LanguageOptions, and modifies the clang Expression evaluator to check the current runtime for those options. If those options are available on the runtime, they are passed to the clang compiler.

Diff Detail

Event Timeline

ldrumm updated this revision to Diff 42853.Dec 15 2015, 7:12 AM
ldrumm retitled this revision from to Add ability to override JIT expr compiler options. .
ldrumm updated this object.
ldrumm added reviewers: clayborg, spyffe.
ldrumm set the repository for this revision to rL LLVM.
ldrumm added a subscriber: lldb-commits.
clayborg requested changes to this revision.Dec 15 2015, 9:57 AM
clayborg edited edge metadata.

See inlined comments.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
85–97 ↗(On Diff #42853)

We have an internal class named StringList that is a vector of strings. It would be nice to use that class instead of a direct std::vector and place this call inside the StringList class as a new method.

191 ↗(On Diff #42853)

We probably shouldn't be using LanguageRuntime, but Language. LanguageRuntime is for things that are dynamically in a process for a given langauge. For example the ObjectiveC language runtime has the dynamic class table of all classes, methods, and other info and this info can be queried from the runtime by looking in the objective C shared library and grabbing stuff from live memory. Language is more for the "this stuff is always this way for this specific language". If you are reading something from memory from the process in order to determine which language options should be enabled, then this would belong in LanguageRuntime. We used to only have lldb_private::LanguageRuntime and we recently added lldb_private::Language, so there might be some stuff that is in the wrong spot inside LangaugeRuntime, but now that we have lldb_private::Language we need to use it correctly.

204 ↗(On Diff #42853)

Should we actually use the frame language if the user specified another language? The user might type:

(lldb) expression --language=swift -- 2+3

and we might be sitting in a C frame. We probably need to use the expression language from the options if it is set.

This revision now requires changes to proceed.Dec 15 2015, 9:57 AM
ldrumm updated this revision to Diff 43616.Dec 24 2015, 1:03 PM
ldrumm edited edge metadata.
ldrumm removed rL LLVM as the repository for this revision.

Updated implementation as per comment on manually set user language: http://reviews.llvm.org/D15527#inline-127860 . This patch adds checking for non eLanguageTypeUnknow expression language before falling back to the language of the current stack frame.

ldrumm updated this revision to Diff 43617.Dec 24 2015, 1:08 PM
ldrumm edited edge metadata.
ldrumm marked 2 inline comments as done.
clayborg accepted this revision.Jan 4 2016, 11:16 AM
clayborg edited edge metadata.

Looks good. Sean should OK this as well.

This revision is now accepted and ready to land.Jan 4 2016, 11:16 AM
ldrumm requested a review of this revision.Jan 12 2016, 3:07 AM
ldrumm edited edge metadata.

bump

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
85–97 ↗(On Diff #42853)

Thanks for the suggestion. I've added that as a separate commit, as it touches an entirely different codepath: http://reviews.llvm.org/D15773

spyffe edited edge metadata.Jan 12 2016, 10:47 AM

I'm a little concerned that LanguageRuntime::GetOverrideExprOptions() appears to generate a new set of options from whole cloth rather than using the existing set as a starting point.

Specifically, since your use case is wanting to override the calling convention, I think it's heavy handed to override all the options. I could conceive of new expression parser features changing some other flags in the future, and it'd be unfortunate if we had to duplicate those features in the ActionScript code just because the options are being overridden.

Could we model this as OverrideExprOptions(clang::TargetOptions &) instead and modify the options in place? That way customizations could stack.

I'm a little concerned that LanguageRuntime::GetOverrideExprOptions() appears to generate a new set of options from whole cloth rather than using the existing set as a starting point.

Specifically, since your use case is wanting to override the calling convention, I think it's heavy handed to override all the options. I could conceive of new expression parser features changing some other flags in the future, and it'd be unfortunate if we had to duplicate those features in the ActionScript code just because the options are being overridden.

That's definitely a good point!

Could we model this as OverrideExprOptions(clang::TargetOptions &) instead and modify the options in place? That way customizations could stack.

I think this is a really helpful suggestion, but I'm slightly foggy on the semantic changes you propose.
If we go ahead, and implement OverrideExprOptions(clang::TargetOptions &) what type do you think the virtual method should return as a default value? I like the semantics of the nullptr meaning "nothing going on here", and I think it's conceptually easier to specialize instances of OverrideExprOptions in individual runtimes with the default base implementation return nullptr in case the user doesn't know or care (though this may be only a personal preference. AFAICS this will also make it easier to pass around pointers to the base class, while having specialized instances available to be constructed in the Runtime dynamically.

I propose that we incorporate your suggestion and pass in a `const clang::TargetOptions & which allows the runtime to learn about its environment, but allows dynamic allocation in subclasses, where this is appropriate. Perhaps also incorporating use of std::shared_ptr` to track ownership of the returned object would be helpful here to improve the management style.

Thanks

Luke

ldrumm updated this revision to Diff 44794.Jan 13 2016, 2:36 PM
ldrumm edited edge metadata.

Responding to Sean Callanan's suggestions in previous differential RE accepting an existing set of TargetOptions as the basis to configure a custom set.

What I'm getting at here is: OverrideExprOptions just subclasses clang::TargetOptions if I'm not mistaken. What I might suggest is

virtual bool
OverrideExprOptions (clang::TargetOptions &options)
{

return false; // return true if you actually modified "options"

}

You'd just modify the compiler's target options in place. That way you only touch the state you care about, and nothing gets dynamically allocated (except the backing stores for the strings in the TargetOptions...)

ldrumm updated this revision to Diff 45588.Jan 21 2016, 1:21 PM

Address @spyffe's previous suggestions regarding modifying target options in place. This allows for a simpler and more maintainable solution than previously implemented, and requires no extra datatype definitions.

  • removed unneccessary SetOverrideTargetOpts method.
  • removed unused OverrideExprOptions type.
  • GetOverrideExprOpts now takes a single clang::TargetOptions reference argument which is modified in-place, and returns a bool value indicating whether the options were modified by the runtime.

Hi Sean

If you have the time I'd appreciate it if you could take another look over my latest iteration of this feature.

Thanks

Luke

I'd like a final review on this patch if one of the code owners can take the time, please?

spyffe accepted this revision.Feb 2 2016, 12:46 PM
spyffe edited edge metadata.

Yeah, this looks much cleaner. I'm fine with this.

This revision is now accepted and ready to land.Feb 2 2016, 12:46 PM
This revision was automatically updated to reflect the committed changes.