This is an archive of the discontinued LLVM Phabricator instance.

Add an option to "break set" and "source list" that takes a line spec in the form file:line:column
ClosedPublic

Authored by jingham on Jul 16 2020, 11:50 AM.

Details

Summary

I think it is important to have separate the file & line specifiers in "break set" because that way no matter what you've named your file, it will always be easy to pass it to lldb. OTOH, for most filenames the "file:line:column" format is unambiguous, and we do print line spec's as "foo.c:10:20" so by rights there should be a way to use that specification.

This patch adds another option to break set (and also source list) that takes a specification of this form. I called it "-y" partly because there aren't that many letters left to use in "break set". Also, y is pretty easy to type, and it's the Spanish equivalent of "and", and since this is a "file AND line" specifier, that provides not too bad of a mnemonic.

For now I reused the source file completer for this, since if you are typing the source file part, that will do the right thing, and we don't do much useful completion on the line & column at present. It would be a nice polish to have another completer that put a ":" after the filename it completes, but I'm not going to do that in this patch. Even nicer would be to have a completer that would complete the line & column entries to only the file/line/column combinations that have line table entries. But that's definitely another patch.

Diff Detail

Event Timeline

jingham created this revision.Jul 16 2020, 11:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2020, 11:50 AM
JDevlieghere added inline comments.Jul 16 2020, 1:03 PM
lldb/include/lldb/Interpreter/OptionValueFileColonLine.h
2

This actually looks like a legit issue.

27

I think these comments add very little, if anything. The override keyword already conveys the same information. Same for the comment below.

44

This should be LLDB_INVALID_COLUMN_NUMBER and probably also be UINT32_MAX unless we rely on it being 0 anywhere?

lldb/include/lldb/lldb-enumerations.h
541

FileLineColumn maybe to match the other enum value?

lldb/source/Interpreter/OptionValue.cpp
474

Can we rename this to eTypeFileLineColumn to make it explicit that it can include all three?

lldb/source/Interpreter/OptionValueFileColonLine.cpp
2

Can you please clang format this file? I know you don't (always) agree but otherwise there are going to be a bunch of unrelated changes when someone else touches (part of) this file.

46

spurious newline?

68

period

75

I think parsing this would be a lot easier with a regex: ([^:]+):(\d+)(:(\d+))?. What do you think?

jingham updated this revision to Diff 278642.Jul 16 2020, 5:36 PM

Address review comments.

jingham marked 8 inline comments as done.Jul 16 2020, 5:42 PM
jingham added inline comments.
lldb/include/lldb/Interpreter/OptionValueFileColonLine.h
27

I removed them here, but note they were just copied from OptionValueFileSpec.h. We started with some header file template that had these way back in the day, and they are all over. If we don't want them around then it would be better to just do that as a patch and not piecemeal.

lldb/source/Interpreter/OptionValueFileColonLine.cpp
68

Ah, nuts. I forgot the period. I'll add it before checking this in.

75

A regex seems overpowered for what I'm doing here, plus that might tempt people to add more stuff to the specifier and I don't want to back into -y being the gdb breakpoint specifier mini-language...

jingham marked 2 inline comments as done.Jul 16 2020, 5:49 PM
jingham added inline comments.
lldb/include/lldb/Interpreter/OptionValueFileColonLine.h
44

The code before this change was using 0 as the invalid value, and there was no define. I added the define and set it to 0, but I haven't propagated that everywhere it is used. That's an orthogonal cleanup which I'd rather do separately.

labath added a subscriber: labath.Jul 17 2020, 5:18 AM
labath added inline comments.
lldb/source/Interpreter/OptionValueFileColonLine.cpp
75

I am not a fan of regex parsing, but I still can't escape the feeling that this should be easier. Maybe a utility function would help:

bool chop_number(StringRef &str, uint32_t &num) {
  auto parts = str.rsplit(':');
  if (to_integer(parts.second, num)) {
    str = parts.first;
    return true;
  }
  return false;
}
...
if (!input.contains(':')
  one_colon_expected();
if (!chop_number(input, col))
  bad_line_number(); // This complains about line numbers because col will be promoted to a line number if the second chop_number fails.
if (!chop_number(input, line)) {
  line = col;
  col = 0;
}
file = input;
121–128

This seems excessive. The command interpreter should already handle the word splitting and quotes, which means that things like br set -y "f i l e.cpp":12:23 will work without this. And it can handle escaping properly, so br set -y "\"file.cpp:12:23" would work, if this code wouldn't get in the way.

130

SetFile already accepts a StringRef.

jingham updated this revision to Diff 278928.Jul 17 2020, 4:21 PM

Address review comments

jingham marked 4 inline comments as done.Jul 17 2020, 4:25 PM
jingham added inline comments.
lldb/source/Interpreter/OptionValueFileColonLine.cpp
75

The only tricky bit was making sure that you got the right error, and were able to show the string that was wrong. I tried your approach but also passing around the found string bit, but it really didn't make anything clearer.

But I reworked it a little to make the logic clearer and added some comments. This version seems pretty straightforward to me.

121–128

I wondered about that too. This is what OptionValueFileSpec did - I started this one from there. So I removed it from both places, and the testsuite is still okay with it.

labath marked an inline comment as done.Jul 20 2020, 2:31 AM

Looks fine to me, modulo the clang-format issues.

lldb/source/Interpreter/OptionValueFileColonLine.cpp
75

Yeah, this looks better. I see now the reason for some of that complexity -- you wanted foo:2b:47 to be considered invalid instead of as "line 47 in file foo:2b -- which seems reasonable.

97

This is one of the reasons I'd recommend llvm::to_integer over StringRef::getAsInteger.

lldb/unittests/Interpreter/TestOptionValueFileColonLine.cpp
38

unused variable

JDevlieghere accepted this revision.Jul 20 2020, 9:51 AM

What Pavel said :-)

This revision is now accepted and ready to land.Jul 20 2020, 9:51 AM
This revision was automatically updated to reflect the committed changes.
jingham marked 2 inline comments as done.