Add ability to override JIT expr compiler options.

[ldrumm]

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.

[ldrumm]

FYI This feature was originally discussed on the ML: http://lists.llvm.org/pipermail/lldb-dev/2015-December/009006.html

[clayborg]

See inlined comments.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

85–97

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

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

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.

[ldrumm]

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.

[clayborg]

Looks good. Sean should OK this as well.

[ldrumm]

bump

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

85–97

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]

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.

[ldrumm]

In D15527#324919, @spyffe wrote:

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]

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

[spyffe]

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]

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.

[ldrumm]

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

[ldrumm]

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

[spyffe]

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

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

 
     // 1. Create a new compiler instance.
     m_compiler.reset(new CompilerInstance());
-    lldb::LanguageType frame_lang = lldb::eLanguageTypeUnknown;
+    lldb::LanguageType frame_lang = expr.Language(); // defaults to lldb::eLanguageTypeUnknown
     lldb_private::LanguageRuntime::OverrideExprOptions *target_opts_override = nullptr;
     lldb_private::LanguageRuntime *lang_rt = nullptr;
     lldb::TargetSP target_sp;

     // If the expression is being evaluated in the context of an existing
     // stack frame, we introspect to see if the language runtime is available.
     auto frame = exe_scope->CalculateStackFrame();
-    if (frame)
+
+    // Make sure the user hasn't provided a preferred execution language
+    // with `expression --language X -- ...`
+    if (frame && frame_lang == lldb::eLanguageTypeUnknown)
         frame_lang = frame->GetLanguage();
 
     if (frame_lang != lldb::eLanguageTypeUnknown)