This is an archive of the discontinued LLVM Phabricator instance.

[Expression] Move IRDynamicChecks to ClangExpressionParser
ClosedPublic

Authored by xiaobai on Jul 11 2019, 1:47 PM.

Details

Summary

IRDynamicChecks in its current form is specific to Clang since it deals
with the C language family. It is possible that we may want to
instrument code generated for other languages, but we can factor in a
more general mechanism to do so at a later time.

This decouples ObCLanguageRuntime from Expression!

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.Jul 11 2019, 1:47 PM
JDevlieghere added inline comments.Jul 11 2019, 2:00 PM
include/lldb/Expression/DynamicCheckerFunctions.h
19 ↗(On Diff #209321)

Can we skip this boilerplate?

27 ↗(On Diff #209321)

Reflow comment.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
1307 ↗(On Diff #209321)

Spurious newline?

xiaobai updated this revision to Diff 209335.Jul 11 2019, 2:23 PM

Reflow comment
Remove newline

xiaobai added inline comments.Jul 11 2019, 2:23 PM
include/lldb/Expression/DynamicCheckerFunctions.h
19 ↗(On Diff #209321)

Do you mean the whole thing? Or just the \class line?

JDevlieghere added inline comments.Jul 11 2019, 2:28 PM
include/lldb/Expression/DynamicCheckerFunctions.h
19 ↗(On Diff #209321)

Everything up to "Encapsulates dynamic check...", Doxygen is smart enough to associate the comment with the class.

xiaobai updated this revision to Diff 209351.Jul 11 2019, 3:02 PM

Remove documentation boilerplate

This is a little unclear to me. LLVMUserExpression is the class that represents "anything that uses LLVM at its back end." Both the Swift & Clang user expressions are instances of this. Running through IR and inserting little callouts is an LLVMUserExpression type thing. So it seems like this move is in the right direction, but to be really clean needs to represent LLVMUserExpression in the Plugin Hierarchy.

This is a little unclear to me. LLVMUserExpression is the class that represents "anything that uses LLVM at its back end." Both the Swift & Clang user expressions are instances of this. Running through IR and inserting little callouts is an LLVMUserExpression type thing. So it seems like this move is in the right direction, but to be really clean needs to represent LLVMUserExpression in the Plugin Hierarchy.

Right, this makes sense to me. My understanding was that LLVMUserExpression would eventually invoke ClangExpressionParser, which actually handles dealing with IR instrumentation. With swift in the picture, things get a little more complicated because you could want to deal with both ObjC code and Swift code. I think that in that case, LLDB would want to use both Clang and Swift to instrument a user expression. I don't think that the abstractions are set up to account for this use case right now, so it would require some planning and refactoring.

Then the IRDynamicCheck part would go with LLVMUserExpression, and the C-specific checks in Clang...

Then the IRDynamicCheck part would go with LLVMUserExpression, and the C-specific checks in Clang...

Except IRDynamicChecks has special knowledge of the clang-specific instrumenters. I can think of another way in which DynamicCheckerFunctions and IRDynamicChecks are more generic classes that will work with any language. I can try to explore that option to see what I come up with, maybe the end result will be better?

jingham accepted this revision.Jul 11 2019, 4:17 PM

That would be cleaner.

OTOH, the original reason for these checkers was to help people understand crashes in their expressions more clearly. Supposedly, modern languages "don't have pointers" and can't have bad objects, so the kind of crashes this instrumentation was supposed to help with "can't happen" and checkers for such languages wouldn't be all that helpful...

So while cleaner, maybe generalizing this more fully isn't a high priority change? In which case, just getting them out of generic code seems fine as a stopping point. Your choice.

This revision is now accepted and ready to land.Jul 11 2019, 4:17 PM

That would be cleaner.

OTOH, the original reason for these checkers was to help people understand crashes in their expressions more clearly. Supposedly, modern languages "don't have pointers" and can't have bad objects, so the kind of crashes this instrumentation was supposed to help with "can't happen" and checkers for such languages wouldn't be all that helpful...

So while cleaner, maybe generalizing this more fully isn't a high priority change? In which case, just getting them out of generic code seems fine as a stopping point. Your choice.

I don't think of it as high priority. That might change at some in the future, but for the time being I think that there are bigger fish to fry.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 5:58 PM
lldb/trunk/include/lldb/Expression/IRDynamicChecks.h