Page MenuHomePhabricator

Add "target.jit-args" settings
ClosedPublic

Authored by endlessroad on Sep 23 2014, 1:59 PM.

Details

Summary

We need an interface to apply compile flags to JIT compiler.

For example, if target is built with -fno-rtti and but JIT compiler does not specify that, JIT'ed code will try to access inexistent RTTI objects. This behavior causes LLVM ExecutionEngine to crash directly.

Diff Detail

Event Timeline

endlessroad retitled this revision from to Add "target.jit-args" settings.
endlessroad updated this object.
endlessroad edited the test plan for this revision. (Show Details)
endlessroad added reviewers: clayborg, tfiala.
endlessroad added a subscriber: Unknown Object (MLST).
tfiala edited edge metadata.EditedSep 30 2014, 9:04 AM

Hey Tong,

Some feedback:

  • The jit-ness of this is more or less an implementation detail. Naming it something related to expression evaluation seems like a better fit. Maybe something like "evaluation-compiler-args".
  • See question on custom handling that one arg in CommandObjectSettings.cpp. That smells wrong. Why is that needed?
  • Can you add a test for this? If it's not tested, you need to assume it's going to be broken. I don't know if you'll end up needing a tiny piece of Android Art compiled code for this? (This might be hard given the size of the exes there - if you can isolate into something very small, it would be good to test against it like we do for the ObjectFileELF* testing).

We'll also want to see if Sean Callanan has anything to add to this since it's in the expression eval pipeline.

Thanks!

source/Commands/CommandObjectSettings.cpp
223 ↗(On Diff #14016)

Hey Tong - why are you needing to special case this one argument here?

endlessroad edited edge metadata.

Address Jim's comments

  • Rename jit-args to expr-parser-compiler-args.
  • Add a common functionality for Args: if one argument is surrounded by quote char, don't consider it as an command option.
jingham edited edge metadata.Sep 30 2014, 1:40 PM

The argument parsing part of this patch doesn't work correctly. It changes all quoted values in the command input from options to arguments For instance, I have this command in my .lldbinit:

type summary add -w lldb "lldb::ModuleSP" -s "${var.__ptr_%S}"

Your patch notices that "${var.__ptr_%S}" has quotes around it, so it removes it from the option parsing. But since the -s requires an argument value, the command no longer parses correctly.

Jim

endlessroad edited edge metadata.

Fix option parsing.

tfiala added a comment.Oct 2 2014, 8:01 AM

Looking at this now.

Initially I was thinking the Expr text could be expanded to Expression, but I see that Env (in EnvVar, ePropertyEnvVars, etc.) is also already similarly abbreviated, so this is not really out of line for that code. So just a minor nit but is probably fine.

Going to test it a bit.

tfiala added a comment.Oct 2 2014, 8:33 AM

I tested this with the lldb docs for 'type summary' here: http://lldb.llvm.org/varformats.html, using the floating point type summary example:

type summary add --summary-string "Sign: ${var[31]%B} Exponent: ${var[30-23]%x} Mantissa: ${var[0-22]%u}" float

That seemed to work in the presence of this change.

The tests come back clean on Ubuntu 14.04 and MacOSX 10.9.5.

Jim, do you have any further comments on this?

clayborg accepted this revision.Oct 6 2014, 3:01 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Oct 6 2014, 3:01 PM

It's ugly to have to add quotes and then remove them, but until we write our own option parser I can't think of a better way to do it...

Jim

tfiala added a comment.Oct 6 2014, 3:59 PM

It's ugly to have to add quotes and then remove them, but until we write

our own option parser I can't think of a better way to do it...

Ok - I'll take that as a "okay for now, let's revisit later" response.

I'll get this in.

tfiala accepted this revision.Oct 6 2014, 4:00 PM
tfiala edited edge metadata.
tfiala closed this revision.Oct 6 2014, 4:23 PM

svn commit
Sending include/lldb/Expression/ClangExpressionParser.h
Sending include/lldb/Target/Target.h
Sending source/Expression/ClangExpressionParser.cpp
Sending source/Expression/ClangFunction.cpp
Sending source/Expression/ClangUserExpression.cpp
Sending source/Expression/ClangUtilityFunction.cpp
Sending source/Interpreter/Args.cpp
Sending source/Target/Target.cpp
Transmitting file data ........
Committed revision 219169.

tfiala added a comment.Oct 8 2014, 9:13 AM

FYI Tong's internship is up, so his @Google.com email address will show up as unreachable.

It looks like this change has introduced this bug:
http://llvm.org/bugs/show_bug.cgi?id=21190

tfiala added a comment.Oct 9 2014, 6:25 PM

I reversed this out with this change:

svn commit
Sending        include/lldb/Expression/ClangExpressionParser.h
Sending        include/lldb/Target/Target.h
Sending        source/Expression/ClangExpressionParser.cpp
Sending        source/Expression/ClangFunction.cpp
Sending        source/Expression/ClangUserExpression.cpp
Sending        source/Expression/ClangUtilityFunction.cpp
Sending        source/Interpreter/Args.cpp
Sending        source/Target/Target.cpp
Transmitting file data ........
Committed revision 219461.

This was due to this bug introduced by the change:
http://llvm.org/bugs/show_bug.cgi?id=21190

Greg had this suggestion to try as a possible way to solve this issue without needing the alternate quote handling:

Just a note for Tong: You can use single or double quotes to get around not being able to quote things:

(lldb) expression --jit-args='-foo "Bar" -baz "123"'

Not sure if that would help Tong's issue.

We'll want to get the actual compiler args back in, just without the quote handling changes.