[lldb][Breakpoint] Allow whitespace in breakpoint address expression #126053
Conversation
Setting a breakpoint on `<symbol> + <offset>` used to work until `2c76e88e9eb284d17cf409851fb01f1d583bb22a`, where this regex was reworked. Now we only accept `<symbol>+ <offset>`. This patch fixes the regression by adding yet another `[[:space:]]*` component to the regex. One could probably simplify the regex (or even replace the regex by just calling the relevent `consumeXXX` APIs on `llvm::StringRef`). Though I left that for the future. rdar://130780342
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesSetting a breakpoint on This patch fixes the regression by adding yet another One could probably simplify the regex (or even replace the regex by just calling the relevent rdar://130780342 Full diff: https://github.com/llvm/llvm-project/pull/126053.diff 1 Files Affected:
diff --git a/lldb/source/Interpreter/OptionArgParser.cpp b/lldb/source/Interpreter/OptionArgParser.cpp
index 800f22b6169dc62..2d393a57452ee56 100644
--- a/lldb/source/Interpreter/OptionArgParser.cpp
+++ b/lldb/source/Interpreter/OptionArgParser.cpp
@@ -262,8 +262,10 @@ OptionArgParser::DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s,
// 3: The symbol/reg name if there is an offset
// 4: +/-
// 5: The offset value.
+ // clang-format off
static RegularExpression g_symbol_plus_offset_regex(
- "^(\\$[^ +-]+)|(([^ +-]+)([-\\+])[[:space:]]*(0x[0-9A-Fa-f]+|[0-9]+)[[:space:]]*)$");
+ "^(\\$[^ +-]+)|(([^ +-]+)[[:space:]]*([-\\+])[[:space:]]*(0x[0-9A-Fa-f]+|[0-9]+)[[:space:]]*)$");
+ // clang-format on
llvm::SmallVector<llvm::StringRef, 4> matches;
if (g_symbol_plus_offset_regex.Execute(sref, &matches)) {
|
|
Can this be tested? |
Oh whoops i forgot to git add the test :) |
| @@ -0,0 +1,3 @@ | |||
| # RUN: %clang_host -g -O0 %S/Inputs/main.c -o %t.out | |||
| # RUN: %lldb -b -o 'breakpoint set -a "main + 26"' %t.out | FileCheck %s --check-prefix CHECK | |||
There was a problem hiding this comment.
You should test a few versions in case the whitespace was ever made mandatory accidentally, on either side of the +.
There was a problem hiding this comment.
Done, will put them all in one lldb invocation
| @@ -0,0 +1,12 @@ | |||
| # RUN: %clang_host -g -O0 %S/Inputs/main.c -o %t.out | |||
| # RUN: %lldb -b -o 'breakpoint set -a "main+26"' \ | |||
There was a problem hiding this comment.
Nested quotes don't work (or maybe work differently) on windows. Better put this into a command file (i.e., this file, since it's just full of comments ).
| static RegularExpression g_symbol_plus_offset_regex( | ||
| "^(\\$[^ +-]+)|(([^ +-]+)([-\\+])[[:space:]]*(0x[0-9A-Fa-f]+|[0-9]+)[[:space:]]*)$"); | ||
| "^(\\$[^ +-]+)|(([^ +-]+)[[:space:]]*([-\\+])[[:space:]]*(0x[0-9A-Fa-f]+|[0-9]+)[[:space:]]*)$"); |
There was a problem hiding this comment.
this is using (in [^ +-])and :space: in the same regex, which seems oddly inconsistent?
Apart from Pavel's comment, this seems fine.
There was a problem hiding this comment.
Yea I think the regex can be simplified. But it's was using :space: already and I didn't want to make drive-by changes to this regex in this PR
There was a problem hiding this comment.
I'd probably prefer to just parse the string if we did want to simplify this logic
…lvm#126053) Setting a breakpoint on `<symbol> + <offset>` used to work until `2c76e88e9eb284d17cf409851fb01f1d583bb22a`, where this regex was reworked. Now we only accept `<symbol>+ <offset>` or `<symbol>+<offset>`. This patch fixes the regression by adding yet another `[[:space:]]*` component to the regex. One could probably simplify the regex (or even replace the regex by just calling the relevent `consumeXXX` APIs on `llvm::StringRef`). Though I left that for the future. rdar://130780342
Setting a breakpoint on
<symbol> + <offset>used to work until2c76e88e9eb284d17cf409851fb01f1d583bb22a, where this regex was reworked. Now we only accept<symbol>+ <offset>or<symbol>+<offset>.This patch fixes the regression by adding yet another
[[:space:]]*component to the regex.One could probably simplify the regex (or even replace the regex by just calling the relevent
consumeXXXAPIs onllvm::StringRef). Though I left that for the future.rdar://130780342