Compiler args patch resubmission

[apepper]

Summary

Resubmission of Tong's compiler args patch (reviews.llvm.org/D5472) with minor modifications.

This patch differs from Tong's original change in how it attempts to match option argument strings for quotation removal after parsing. The original change compared strings which could fail when more than one string matched, this version checks the string pointer returned from the parser with the original string in the argv array. This change was driven by the issue reported in Bug 21190. Ideally this would be resolved by writing a different options parser but until that happens this is a workable solution.

[emaste]

On FreeBSD this change introduced 29 test failures. It seems like they are all segfaults.

My test build was w/o debug info - I'll have to rebuild and grab a backtrace.

Failing Tests (29)
FAIL: LLDB (suite) :: TestHelloWorld.py (FreeBSD feynman 10.0-STABLE FreeBSD 10.0-STABLE #1 r268946+94ba9c8(stable-10): Mon Jul 21 14:06:09 EDT 2014     emaste@feynman:/tank/emaste/obj/tank/emaste/src/git-stable-10/sys/GENERIC amd64 amd64)
FAIL: LLDB (suite) :: TestMultithreaded.py (FreeBSD feynman 10.0-STABLE FreeBSD 10.0-STABLE #1 r268946+94ba9c8(stable-10): Mon Jul 21 14:06:09 EDT 2014     emaste@feynman:/tank/emaste/obj/tank/emaste/src/git-stable-10/sys/GENERIC amd64 amd64)
FAIL: LLDB (suite) :: TestConditionalBreak.py (FreeBSD feynman 10.0-STABLE FreeBSD 10.0-STABLE #1 r268946+94ba9c8(stable-10): Mon Jul 21 14:06:09 EDT 2014     emaste@feynman:/tank/emaste/obj/tank/emaste/src/git-stable-10/sys/GENERIC amd64 amd64)
FAIL: LLDB (suite) :: TestBreakpointCommand.py (FreeBSD feynman 10.0-STABLE FreeBSD 10.0-STABLE #1 r268946+94ba9c8(stable-10): Mon Jul 21 14:06:09 EDT 2014     emaste@feynman:/tank/emaste/obj/tank/emaste/src/git-stable-10/sys/GENERIC amd64 amd64)
...
ninja: build stopped: subcommand failed.

[apepper]

I am looking into it. Thanks.

• Alex

[emaste]

In D5806#84283, @apepper wrote:

I am looking into it. Thanks.

• Alex

Still planning a change along these lines?

[emaste]

Ping?

[emaste]

Please feel free to re-add me if there's a new patch to review; I'm trying to clear out my outstanding review list.

[tfiala]

Stale and not happening AFAICT.

include/lldb/Expression/ClangExpressionParser.h

     //------------------------------------------------------------------
     ClangExpressionParser (ExecutionContextScope *exe_scope,
                            ClangExpression &expr,
+                           Args &expr_parser_compiler_args,
                            bool generate_debug_info);
     
     //------------------------------------------------------------------

include/lldb/Target/Target.h

     
     void
     SetRunArguments (const Args &args);
+
+    bool
+    GetExprParserCompilerArguments (Args &args) const;
+
+    void
+    SetExprParserCompilerArguments (const Args &args);
     
     size_t
     GetEnvironmentAsArgs (Args &env) const;

source/Expression/ClangExpressionParser.cpp

 
 ClangExpressionParser::ClangExpressionParser (ExecutionContextScope *exe_scope,
                                               ClangExpression &expr,
+                                              Args &expr_parser_compiler_args,
                                               bool generate_debug_info) :
     m_expr (expr),
     m_compiler (),

 
     // 3. Set options.
 
+    clang::CompilerInvocation::CreateFromArgs(m_compiler->getInvocation(),
+        expr_parser_compiler_args.GetConstArgumentVector(),
+        expr_parser_compiler_args.GetConstArgumentVector() + expr_parser_compiler_args.GetArgumentCount(),
+        m_compiler->getDiagnostics());
+
     lldb::LanguageType language = expr.Language();
 
     switch (language)

source/Expression/ClangFunction.cpp

     if (jit_process_sp)
     {
         const bool generate_debug_info = true;
-        m_parser.reset(new ClangExpressionParser(jit_process_sp.get(), *this, generate_debug_info));
+        Args expr_parser_compiler_args;
+        jit_process_sp->GetTarget().GetExprParserCompilerArguments (expr_parser_compiler_args);
+        m_parser.reset(new ClangExpressionParser(jit_process_sp.get(), *this, expr_parser_compiler_args, generate_debug_info));
         
         num_errors = m_parser->Parse (errors);
     }

source/Expression/ClangUserExpression.cpp

     if (!exe_scope)
         exe_scope = exe_ctx.GetTargetPtr();
 
-    ClangExpressionParser parser(exe_scope, *this, generate_debug_info);
+    Args expr_parser_compiler_args;
+    target->GetExprParserCompilerArguments (expr_parser_compiler_args);
+    ClangExpressionParser parser(exe_scope, *this, expr_parser_compiler_args, generate_debug_info);
 
     unsigned num_errors = parser.Parse (error_stream);
 

source/Expression/ClangUtilityFunction.cpp

     }
     
     const bool generate_debug_info = true;
-    ClangExpressionParser parser(exe_ctx.GetBestExecutionContextScope(), *this, generate_debug_info);
+    Args expr_parser_compiler_args;
+    target->GetExprParserCompilerArguments (expr_parser_compiler_args);
+    ClangExpressionParser parser(exe_ctx.GetBestExecutionContextScope(), *this, expr_parser_compiler_args, generate_debug_info);
     
     unsigned num_errors = parser.Parse (error_stream);
     

source/Interpreter/Args.cpp

         }
     }
     OptionParser::Prepare();
+
+    // Before parsing arguments, insert quote char to the head of the string.
+    // So quoted arguments like "-l" won't be treated as option.
+    int argv_iter = 0;
+    for (auto args_iter = m_args.begin(); args_iter != m_args.end(); args_iter++, argv_iter++)
+    {
+        char quote_char = GetArgumentQuoteCharAtIndex(argv_iter);
+        if (quote_char != '\0')
+        {
+            *args_iter = std::string(1, quote_char) + *args_iter;
+            m_argv[argv_iter] = args_iter->c_str();
+        }
+    }
+
     int val;
     while (1)
     {

             }
             else
             {
+                const char *value = OptionParser::GetOptionArgument();
+                if (value)
+                {
+                    // Remove leading quote char from option value
+                    argv_iter = 0;
+                    for (auto args_iter = m_args.begin(); args_iter != m_args.end(); args_iter++, argv_iter++)
+                    {
+                        // Compare the option argument pointer returned against the original list to
+                        // ensure we are checking the correct option argument string for quotes
+                        if (m_argv[argv_iter] == value && GetArgumentQuoteCharAtIndex(argv_iter) != '\0')
+                        {
+                            *args_iter = args_iter->substr(1);
+                            value = args_iter->c_str();
+                            break;
+                        }
+                    }
+                }
+
                 error = options.SetOptionValue(long_options_index,
                                                (def->option_has_arg == OptionParser::eNoArgument) ? nullptr : OptionParser::GetOptionArgument());
             }

     // Update our ARGV now that get options has consumed all the options
     m_argv.erase(m_argv.begin(), m_argv.begin() + OptionParser::GetOptionIndex());
     UpdateArgsAfterOptionParsing ();
+
+    // Remove leading quote char from other arguments.
+    argv_iter = 0;
+    for (auto args_iter = m_args.begin(); args_iter != m_args.end(); args_iter++, argv_iter++)
+    {
+        char quote_char = GetArgumentQuoteCharAtIndex(argv_iter);
+        if (quote_char != '\0')
+        {
+            *args_iter = args_iter->substr(1);
+            m_argv[argv_iter] = args_iter->c_str();
+        }
+    }
+
     return error;
 }
 

source/Target/Target.cpp

     { "breakpoints-use-platform-avoid-list", OptionValue::eTypeBoolean   , false, true                      , NULL, NULL, "Consult the platform module avoid list when setting non-module specific breakpoints." },
     { "arg0"                               , OptionValue::eTypeString    , false, 0                         , NULL, NULL, "The first argument passed to the program in the argument array which can be different from the executable itself." },
     { "run-args"                           , OptionValue::eTypeArgs      , false, 0                         , NULL, NULL, "A list containing all the arguments to be passed to the executable when it is run. Note that this does NOT include the argv[0] which is in target.arg0." },
+    { "expr-parser-compiler-args"          , OptionValue::eTypeArgs      , false, 0                         , NULL, NULL, "A list containing all the arguments to be passed to the expression parser compiler." },
     { "env-vars"                           , OptionValue::eTypeDictionary, false, OptionValue::eTypeString  , NULL, NULL, "A list of all the environment variables to be passed to the executable's environment, and their values." },
     { "inherit-env"                        , OptionValue::eTypeBoolean   , false, true                      , NULL, NULL, "Inherit the environment from the process that is running LLDB." },
     { "input-path"                         , OptionValue::eTypeFileSpec  , false, 0                         , NULL, NULL, "The file/path to be used by the executable program for reading its standard input." },

     ePropertyBreakpointUseAvoidList,
     ePropertyArg0,
     ePropertyRunArgs,
+    ePropertyExprParserCompilerArgs,
     ePropertyEnvVars,
     ePropertyInheritEnv,
     ePropertyInputPath,

 }
 
 void
+TargetProperties::SetExprParserCompilerArguments (const Args &args)
+{
+    const uint32_t idx = ePropertyExprParserCompilerArgs;
+    m_collection_sp->SetPropertyAtIndexFromArgs (NULL, idx, args);
+}
+
+bool
+TargetProperties::GetExprParserCompilerArguments (Args &args) const
+{
+    const uint32_t idx = ePropertyExprParserCompilerArgs;
+    return m_collection_sp->GetPropertyAtIndexAsArgs (NULL, idx, args);
+}
+
+void
 TargetProperties::SetRunArguments (const Args &args)
 {
     const uint32_t idx = ePropertyRunArgs;