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.

include/lldb/Target/LanguageRuntime.h

 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Target/ExecutionContextScope.h"
+#include "clang/Basic/TargetOptions.h"
 
 namespace lldb_private {
 

 {
 public:
 
+    class OverrideExprOptions : public clang::TargetOptions {};
     ~LanguageRuntime() override;
     
     static LanguageRuntime* 

     {
     }
 
+    virtual OverrideExprOptions *
+    GetOverrideExprOptions()
+    {
+        return nullptr;
+    }
+
+    virtual bool
+    SetOverrideExprOptions(OverrideExprOptions *opts)
+    {
+        return false;
+    }
+
 protected:
     //------------------------------------------------------------------
     // Classes that inherit from LanguageRuntime can see and modify these

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

 #include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Target/Language.h"
 
 using namespace clang;
 using namespace llvm;
 using namespace lldb_private;
 
+namespace {
+    void debugStringVector(Log *log, const std::vector<std::string>& vec, const char *name)
+    {
+        if(!log)
+            return;
+
+        log->Debug("Begin %s:", name);
+        for (const auto& s : vec)
+            log->Debug("%s", s.c_str());
+
+        log->Debug("End %s.", name);
+    }
+}
+
+
 //===----------------------------------------------------------------------===//
 // Utility Methods for Clang
 //===----------------------------------------------------------------------===//

     m_code_generator (),
     m_pp_callbacks(nullptr)
 {
+    Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_EXPRESSIONS));
+
     // 1. Create a new compiler instance.
     m_compiler.reset(new CompilerInstance());
-
-    // 2. Install the target.
-
+    lldb::LanguageType frame_lang = lldb::eLanguageTypeUnknown;
+    lldb_private::LanguageRuntime::OverrideExprOptions *target_opts_override = nullptr;
+    lldb_private::LanguageRuntime *lang_rt = nullptr;
     lldb::TargetSP target_sp;
     if (exe_scope)
         target_sp = exe_scope->CalculateTarget();
 
-    // TODO: figure out what to really do when we don't have a valid target.
-    // Sometimes this will be ok to just use the host target triple (when we
-    // evaluate say "2+3", but other expressions like breakpoint conditions
-    // and other things that _are_ target specific really shouldn't just be
-    // using the host triple. This needs to be fixed in a better way.
-    if (target_sp && target_sp->GetArchitecture().IsValid())
+    // 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)
+        frame_lang = frame->GetLanguage();
+
+    if (frame_lang != lldb::eLanguageTypeUnknown)
     {
-        std::string triple = target_sp->GetArchitecture().GetTriple().str();
-        m_compiler->getTargetOpts().Triple = triple;
+        lang_rt = exe_scope->CalculateProcess()->GetLanguageRuntime(frame_lang);
+        if (log)
+            log->Printf("Frame has language of type %s", Language::GetNameForLanguageType(frame_lang));
     }
-    else
+
+    // 2. Configure the target, overriding with any custom options we can get.
+    if (lang_rt)
     {
-        m_compiler->getTargetOpts().Triple = llvm::sys::getDefaultTargetTriple();
+        // The expression is being evaluated in a known LanguageRuntime and StackFrame.
+        // We check if there are any overridden options for the evaluation, and configure
+        // the compiler appropriately.
+        target_opts_override = lang_rt->GetOverrideExprOptions();
     }
 
-    if (target_sp->GetArchitecture().GetMachine() == llvm::Triple::x86 ||
-        target_sp->GetArchitecture().GetMachine() == llvm::Triple::x86_64)
+    if (target_opts_override)
     {
-        m_compiler->getTargetOpts().Features.push_back("+sse");
-        m_compiler->getTargetOpts().Features.push_back("+sse2");
+        if (log)
+            log->Debug("Using overridden target options for the expression evaluation");
+
+        // If we get here this means the given language runtime has a custom set of code
+        // generation requirements, and they are configured from the language runtime.
+        m_compiler->getTargetOpts().Features = target_opts_override->Features;
+        m_compiler->getTargetOpts().CPU = target_opts_override->CPU;
+        m_compiler->getTargetOpts().Triple = target_opts_override->Triple;
+        m_compiler->getTargetOpts().FeaturesAsWritten = target_opts_override->FeaturesAsWritten;
+        m_compiler->getTargetOpts().Reciprocals = target_opts_override->Reciprocals;
+    }
+    else
+    {
+        // In this case, a specialized language runtime is not available and
+        // we have to fallback to making some basic assumptions on the target architecture.
+        // For 99% of use cases, this will be fine and is assumed to be the default.
+        if (target_sp && target_sp->GetArchitecture().IsValid())
+        {
+            std::string triple = target_sp->GetArchitecture().GetTriple().str();
+            m_compiler->getTargetOpts().Triple = triple;
+            if (log)
+                log->Printf("Using %s as the target triple", m_compiler->getTargetOpts().Triple.c_str());
+        }
+        else
+        {
+            // If we get here we don't have a valid target and just have to guess.
+            // Sometimes this will be ok to just use the host target triple (when we
+            // evaluate say "2+3", but other expressions like breakpoint conditions
+            // and other things that _are_ target specific really shouldn't just be
+            // using the host triple. In such a case the language runtime should
+            // expose an overridden options set rather than letting this default path execute.
+            m_compiler->getTargetOpts().Triple = llvm::sys::getDefaultTargetTriple();
+            if (log)
+                log->Printf("Using default target triple of %s", m_compiler->getTargetOpts().Triple.c_str());
+        }
+        // Now add some special fixes for known architectures:
+        // Any arm32 iOS environment, but not on arm64
+        if (m_compiler->getTargetOpts().Triple.find("arm64") == std::string::npos &&
+            m_compiler->getTargetOpts().Triple.find("arm") != std::string::npos &&
+            m_compiler->getTargetOpts().Triple.find("ios") != std::string::npos)
+        {
+            m_compiler->getTargetOpts().ABI = "apcs-gnu";
+        }
+        // Supported subsets of x86
+        if (target_sp->GetArchitecture().GetMachine() == llvm::Triple::x86 ||
+            target_sp->GetArchitecture().GetMachine() == llvm::Triple::x86_64)
+        {
+            m_compiler->getTargetOpts().Features.push_back("+sse");
+            m_compiler->getTargetOpts().Features.push_back("+sse2");
+        }
     }
 
-    // Any arm32 iOS environment, but not on arm64
-    if (m_compiler->getTargetOpts().Triple.find("arm64") == std::string::npos &&
-        m_compiler->getTargetOpts().Triple.find("arm") != std::string::npos &&
-        m_compiler->getTargetOpts().Triple.find("ios") != std::string::npos)
+    if (log)
     {
-        m_compiler->getTargetOpts().ABI = "apcs-gnu";
+        auto opts = m_compiler->getTargetOpts();
+        log->Debug("Triple: '%s'", opts.Triple.c_str());
+        log->Debug("CPU: '%s'", opts.CPU.c_str());
+        log->Debug("FPMath: '%s'", opts.FPMath.c_str());
+        log->Debug("ABI: '%s'", opts.ABI.c_str());
+        log->Debug("LinkerVersion: '%s'", opts.LinkerVersion.c_str());
+        debugStringVector(log, opts.FeaturesAsWritten, "FeaturesAsWritten");
+        debugStringVector(log, opts.Features, "Features");
+        debugStringVector(log, opts.Reciprocals, "Reciprocals");
     }
 
+    // 3. Create and install the target on the compiler.
     m_compiler->createDiagnostics();
-
-    // Create the target instance.
-    m_compiler->setTarget(TargetInfo::CreateTargetInfo(
-        m_compiler->getDiagnostics(), m_compiler->getInvocation().TargetOpts));
+    auto target_info = TargetInfo::CreateTargetInfo(m_compiler->getDiagnostics(), m_compiler->getInvocation().TargetOpts);
+    if (log)
+    {
+        log->Printf("Using SIMD alignment: %d", target_info->getSimdDefaultAlign());
+        log->Printf("Target datalayout string: '%s'", target_info->getDataLayoutString());
+        log->Printf("Target ABI: '%s'", target_info->getABI().str().c_str());
+        log->Printf("Target vector alignment: %d", target_info->getMaxVectorAlign());
+    }
+    m_compiler->setTarget(target_info);
 
     assert (m_compiler->hasTarget());
 
-    // 3. Set options.
+    // 4. Set language options.
 
     lldb::LanguageType language = expr.Language();
 

     // created. This complexity should be lifted elsewhere.
     m_compiler->getTarget().adjust(m_compiler->getLangOpts());
 
-    // 4. Set up the diagnostic buffer for reporting errors
+    // 5. Set up the diagnostic buffer for reporting errors
 
     m_compiler->getDiagnostics().setClient(new clang::TextDiagnosticBuffer);
 
-    // 5. Set up the source management objects inside the compiler
+    // 6. Set up the source management objects inside the compiler
 
     clang::FileSystemOptions file_system_options;
     m_file_manager.reset(new clang::FileManager(file_system_options));

         m_compiler->getPreprocessor().addPPCallbacks(std::move(pp_callbacks));
     }
         
-    // 6. Most of this we get from the CompilerInstance, but we
+    // 7. Most of this we get from the CompilerInstance, but we
     // also want to give the context an ExternalASTSource.
     m_selector_table.reset(new SelectorTable());
     m_builtin_context.reset(new Builtin::Context());