Page MenuHomePhabricator

[lldb] Add option to retry Fix-Its multiple times to failed expressions
ClosedPublic

Authored by teemperor on Apr 1 2020, 6:32 AM.

Details

Summary

Usually when Clang emits an error Fix-It it does two things. It emits the diagnostic and then it fixes the
currently generated AST to reflect the applied Fix-It. While emitting the diagnostic is easy to implement,
fixing the currently generated AST is often tricky. That causes that some Fix-Its just keep the AST as-is or
abort the parsing process entirely. Once the parser stopped, any Fix-Its for the rest of the expression are
not detected and when the user manually applies the Fix-It, the next expression will just produce a new
Fix-It.

This is often occurring with quickly made Fix-Its that are just used to bridge temporary API changes
and that often are not worth implementing a proper API fixup in addition to the diagnostic. To still
give some kind of reasonable user-experience for users that have these Fix-Its and rely on them to
fix their expressions, this patch adds the ability to retry parsing with applied Fix-Its multiple time to
give the normal Fix-It experience where things Clang knows how to fix are not causing actual expression
error (at least when automatically applying Fix-Its is activated).

The way this is implemented is just by having another setting in the expression options that specify how
often we should try applying Fix-Its and then reparse the expression. The default setting is still 1 for everyone
so this should not affect the speed in which we fail to parse expressions.

Diff Detail

Event Timeline

teemperor created this revision.Apr 1 2020, 6:32 AM
shafik accepted this revision.Apr 1 2020, 9:12 AM
shafik added a subscriber: shafik.
shafik added inline comments.
lldb/source/Expression/UserExpression.cpp
269–295

Why not uint32_t?

lldb/source/Target/Target.cpp
3715

Why not uint32_t?

This revision is now accepted and ready to land.Apr 1 2020, 9:12 AM
teemperor updated this revision to Diff 254518.Apr 2 2020, 7:24 AM

Yeah the 32/64 mix is because the options don't directly support uint32_t values while the SBAPI heavily prefers uint32_t over uint64_t parameter. But there are some parts of the SB API that use uint64_t so I think the bets way out of this is to just use uint64_t consistently everywhere.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 2:41 AM
omjavaid reopened this revision.Apr 7 2020, 2:58 AM
omjavaid added a subscriber: omjavaid.

This patch fails on lldb-aarch64-linux with following backtrace, apparently failing experession eval. I am marking it xfail for now.

Traceback (most recent call last):

File "/home/omair.javaid/work/lldb/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1751, in test_method
  return attrvalue(self)
File "/home/omair.javaid/work/lldb/llvm-project/lldb/test/API/commands/expression/fixits/TestFixIts.py", line 151, in test_with_target
  self.expect_expr("test_X(1)", result_type="int", result_value="123")
File "/home/omair.javaid/work/lldb/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2426, in expect_expr
  "Unexpected failure with msg: " + eval_result.GetError().GetCString())

AssertionError: False is not True : Unexpected failure with msg: error: Execution was interrupted, reason: signal SIGILL: illegal instruction.
The process has been returned to the state before expression evaluation.

This revision is now accepted and ready to land.Apr 7 2020, 2:58 AM
teemperor closed this revision.Apr 13 2020, 4:26 AM

Somehow Phab forget to close this one.