This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Refactor argument group by SourceLocationSpec (NFCI)
ClosedPublic

Authored by mib on Apr 21 2021, 8:38 AM.

Details

Summary

This patch refactors a good part of the code base turning the usual
FileSpec, Line, Column, CheckInlines, ExactMatch arguments into a
SourceLocationSpec object.

This change is required for a following patch that will add handling of the
column line information when doing symbol resolution.

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Apr 21 2021, 8:38 AM
mib requested review of this revision.Apr 21 2021, 8:38 AM
mib updated this revision to Diff 339255.Apr 21 2021, 8:40 AM
mib edited reviewers, added: teemperor, jingham; removed: jdoerfert.

Nice, it's great to see functions taking less arguments for a change ;-)

lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
12–15
30

Was this meant to be a const ref? There's no point in making objects passed by value const, as they're already copied in and this just means that if the function wants to modify them, they have to make a second copy while the function already has its own copy.

lldb/include/lldb/Core/AddressResolverFileLine.h
43–44

It seems like most of the places where you use the SourceLocationSpec, you also pass "check_inlines" and "exact". Those seem to me natural parts of a source location search specification class, and including them in the Spec would clean up the API's even further. Not necessary if it gets ugly for some reason, but it would look nicer.

mib updated this revision to Diff 339878.Apr 22 2021, 10:06 PM
mib marked 3 inline comments as done.

Address @JDevlieghere & @jingham feedbacks.

mib retitled this revision from [lldb] Refactor (FileSpec+Line) to SourceLocationSpec (NFCI) to [lldb] Refactor argument group by SourceLocationSpec (NFCI).Apr 22 2021, 10:07 PM
mib edited the summary of this revision. (Show Details)
mib updated this revision to Diff 339881.Apr 22 2021, 10:16 PM

Update LineEntry unit test.

mib updated this revision to Diff 339882.Apr 22 2021, 10:23 PM

Update BreakpointResolver column default value and fix typo

JDevlieghere requested changes to this revision.Apr 23 2021, 8:43 AM

In several places you have SourceLocationSpec::Create followed by a lldbassert. That's not sufficient, because in a non-assert build, the lldbassert is going to print a message but not halt the program and we'll crash when trying to dereference the expected. You'll need to add proper error handling to those places to bail out if the location spec is not valid.

lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
63

Move this before the bool to reduce padding.

lldb/source/API/SBThread.cpp
856

This will crash in a non-assert build. You can't rely on the assert to make this unreachable.

lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
274–275

I'd get the column, check if it's valid, and use it, instead of calling HasColumn before GetColumn.

lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
116

Same here

This revision now requires changes to proceed.Apr 23 2021, 8:43 AM
shafik added a subscriber: shafik.Apr 23 2021, 10:28 AM
shafik added inline comments.
lldb/source/API/SBThread.cpp
852
step_file_spec, line, /*column*/llvm::None, check_inlines, exact);
lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
114
cu_file_spec, line_matches[i], /*column*/llvm::None, search_inlines, m_exact_match);
mib updated this revision to Diff 340128.Apr 23 2021, 11:54 AM
mib marked 6 inline comments as done.

Addressed @JDevlieghere @shafik feedbacks.

JDevlieghere added inline comments.Apr 23 2021, 12:31 PM
lldb/source/API/SBThread.cpp
852

As @teemperor pointed out in another patch, the llvm style includes a = at the end (https://www.llvm.org/docs/CodingStandards.html#comment-formatting)

auto location_spec = SourceLocationSpec::Create(
        step_file_spec, line, /*colum=*/llvm::None, check_inlines, exact);
854–857

Let's move Invalid SourceLocationSpec: (without the capitalization) into the error reported by SourceLocationSpec::Create and use the Status ctor that takes an llvm::Error to avoid this code duplication.

mib marked an inline comment as done.Apr 23 2021, 3:27 PM
mib updated this revision to Diff 340183.Apr 23 2021, 3:30 PM

Addressed @JDevlieghere comments.

mib updated this revision to Diff 340211.Apr 23 2021, 5:30 PM

Fix test failures.

JDevlieghere added inline comments.Apr 23 2021, 5:58 PM
lldb/source/API/SBThread.cpp
854

Why not sb_error = location_spec.takeError()?

mib marked an inline comment as done.Apr 23 2021, 6:29 PM
mib added inline comments.
lldb/source/API/SBThread.cpp
854

takeError returns an llvm::Error not a lldb_private::Status, and there is no SBError constructor for Status so I don't think that's possible.

JDevlieghere added inline comments.Apr 23 2021, 7:17 PM
lldb/source/API/SBThread.cpp
854

There is:

explicit Status(llvm::Error error) { *this = std::move(error); }
const Status &operator=(llvm::Error error);

I am wondering how SourceLocationSpec is related to lldb's Declaration class (which is FileSpec + line + column and describes a location in source code)? It seems like the current SourceLocationSpec is just a Declaration with the two additional search variables (and the first iteration was exactly the same as Declaration).

Could we maybe turn SourceLocationSpec to store a lldb::Declaration instead of file/line/column? I'm aware that the Declaration class first needs some cleanup (and a removal of that #ifdef around column....), but right now we are already using Declarations in a bunch of places to describe source locations.

mib updated this revision to Diff 340701.Apr 26 2021, 6:05 PM
mib marked 2 inline comments as done.

Use llvm::Expected error when possible.

mib added a comment.EditedApr 26 2021, 6:14 PM

I am wondering how SourceLocationSpec is related to lldb's Declaration class (which is FileSpec + line + column and describes a location in source code)? It seems like the current SourceLocationSpec is just a Declaration with the two additional search variables (and the first iteration was exactly the same as Declaration).
Could we maybe turn SourceLocationSpec to store a lldb::Declaration instead of file/line/column? I'm aware that the Declaration class first needs some cleanup (and a removal of that #ifdef around column....), but right now we are already using Declarations in a bunch of places to describe source locations.

I don't think adding a level of indirection would serve much purpose and I don't like the fact that Declaration is an exposed class (being part of the lldb namespace), and that it uses a macro-based system for invalid values. Also SourceLocationSpec instances are immutable and must be valid. May be I could try to replace all occurrences of Declaration by SourceLocationSpec in a follow-up patch ?

jingham added a comment.EditedApr 27 2021, 10:17 AM

I am wondering how SourceLocationSpec is related to lldb's Declaration class (which is FileSpec + line + column and describes a location in source code)? It seems like the current SourceLocationSpec is just a Declaration with the two additional search variables (and the first iteration was exactly the same as Declaration).

Could we maybe turn SourceLocationSpec to store a lldb::Declaration instead of file/line/column? I'm aware that the Declaration class first needs some cleanup (and a removal of that #ifdef around column....), but right now we are already using Declarations in a bunch of places to describe source locations.

Seems to me Declarations and SourceLocationSpec's are different things. A Declaration describes just where something is in a source file. And moreover, there need to be a lot of them, since every function, type etc has one. So you might be concerned about size for this entity.

A SourceLocationSpec (maybe we should make a better name?) is about specifying how you search for a source location (e.g. to set a breakpoint on it.) And there are never going to be lots of them in flight, since they are tied to user actions. So we are fairly free to add extra fields to this if we have other ways of specifying the match to a source location. OTOH, Declarations don't need "include_inlines" or "exact_match" or anything else we might end up adding to specify how to look for matches against a source line specification somebody used in break set or other places.

So I don't think it makes sense to conflate the two.

JDevlieghere requested changes to this revision.Apr 27 2021, 6:28 PM
JDevlieghere added inline comments.
lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
234
lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
117

We can't write straight to stderr. We should either have this return an Expected<Searcher::CallbackReturn> or deal with the error in some other way.

lldb/source/Core/Module.cpp
609

Same here

935

and here

lldb/source/Symbol/CompileUnit.cpp
234

and here

This revision now requires changes to proceed.Apr 27 2021, 6:28 PM

I am wondering how SourceLocationSpec is related to lldb's Declaration class (which is FileSpec + line + column and describes a location in source code)? It seems like the current SourceLocationSpec is just a Declaration with the two additional search variables (and the first iteration was exactly the same as Declaration).

Could we maybe turn SourceLocationSpec to store a lldb::Declaration instead of file/line/column? I'm aware that the Declaration class first needs some cleanup (and a removal of that #ifdef around column....), but right now we are already using Declarations in a bunch of places to describe source locations.

Seems to me Declarations and SourceLocationSpec's are different things. A Declaration describes just where something is in a source file. And moreover, there need to be a lot of them, since every function, type etc has one. So you might be concerned about size for this entity.

A SourceLocationSpec (maybe we should make a better name?) is about specifying how you search for a source location (e.g. to set a breakpoint on it.) And there are never going to be lots of them in flight, since they are tied to user actions. So we are fairly free to add extra fields to this if we have other ways of specifying the match to a source location. OTOH, Declarations don't need "include_inlines" or "exact_match" or anything else we might end up adding to specify how to look for matches against a source line specification somebody used in break set or other places.

So I don't think it makes sense to conflate the two.

My point was that Declaration should be a member of SourceLocationSpec (which will then be a Declaration + the search parameters). I agree that those two should stay two different classes. From what understand from Ismail this patch is (was?) going in the direction you're describing of having SourceLocationSpec taking over Declaration (which would then raise the memory concerns you mentioned).

In other words: If the file/line/column members could just be a Declaration (assuming we remove that ifdef around Declarations column member) then IMHO this would be nicer. I would also like the idea of maybe typedef'ing the line/column member types in addition to that. Not sure if we need uint32_t for columns or maybe we need one day uint64_t for lines.

I am wondering how SourceLocationSpec is related to lldb's Declaration class (which is FileSpec + line + column and describes a location in source code)? It seems like the current SourceLocationSpec is just a Declaration with the two additional search variables (and the first iteration was exactly the same as Declaration).

Could we maybe turn SourceLocationSpec to store a lldb::Declaration instead of file/line/column? I'm aware that the Declaration class first needs some cleanup (and a removal of that #ifdef around column....), but right now we are already using Declarations in a bunch of places to describe source locations.

Seems to me Declarations and SourceLocationSpec's are different things. A Declaration describes just where something is in a source file. And moreover, there need to be a lot of them, since every function, type etc has one. So you might be concerned about size for this entity.

A SourceLocationSpec (maybe we should make a better name?) is about specifying how you search for a source location (e.g. to set a breakpoint on it.) And there are never going to be lots of them in flight, since they are tied to user actions. So we are fairly free to add extra fields to this if we have other ways of specifying the match to a source location. OTOH, Declarations don't need "include_inlines" or "exact_match" or anything else we might end up adding to specify how to look for matches against a source line specification somebody used in break set or other places.

So I don't think it makes sense to conflate the two.

My point was that Declaration should be a member of SourceLocationSpec (which will then be a Declaration + the search parameters). I agree that those two should stay two different classes. From what understand from Ismail this patch is (was?) going in the direction you're describing of having SourceLocationSpec taking over Declaration (which would then raise the memory concerns you mentioned).

In other words: If the file/line/column members could just be a Declaration (assuming we remove that ifdef around Declarations column member) then IMHO this would be nicer. I would also like the idea of maybe typedef'ing the line/column member types in addition to that. Not sure if we need uint32_t for columns or maybe we need one day uint64_t for lines.

Ah, I see. Yes, that's reasonable. Then when source files get 3-dimensional we can add in the z-axis column w/o having to change our API's!

mib marked 5 inline comments as done.Apr 29 2021, 8:11 PM
mib updated this revision to Diff 341755.EditedApr 29 2021, 8:16 PM

As discussed with @JDevlieghere offline, at the moment SearchFilter and Searcher (from which many class inherit) are not designed for error handling so I removed the SourceLocationSpec::Create factory method and added an IsValid method to check the state optionally.

mib added a comment.Apr 29 2021, 8:18 PM

The points that @teemperor brought up have been implemented in D101556 and D100962.

mib updated this revision to Diff 341766.Apr 29 2021, 9:35 PM

A few nits but overall this looks like a great cleanup!

lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
260–261

Would it make sense for the column here to be passed as an optional. Right now you're unpacking it which means we potentially have to check for LLDB_INVALID_COLUMN again in the function that's called here.

lldb/source/Core/Module.cpp
603–605

nit: This probably won't matter after the todo, but we should be consistent between using the comment or a named variable. Same observation below.

mib updated this revision to Diff 342079.Apr 30 2021, 4:21 PM
mib marked 2 inline comments as done.
JDevlieghere accepted this revision.May 3 2021, 2:34 PM

LGTM

lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
271–277

I was going to suggest calling the location spec's dump method, but I assume we don't want to change the format. Would it make sense to have a GetDescription in the SourceLocationSpec class or change the dump format to match this format?

This revision is now accepted and ready to land.May 3 2021, 2:34 PM
This revision was landed with ongoing or failed builds.May 4 2021, 4:05 PM
This revision was automatically updated to reflect the committed changes.