This is an archive of the discontinued LLVM Phabricator instance.

Further refinements on reporting interruption in lldb
ClosedPublic

Authored by jingham on Jul 5 2023, 1:10 PM.

Details

Summary

This patch enhances the interruption features I added a little while back. There are two main changes. The main one here is that I made a nicer interface to reporting interruption events. I also added a way to ask a debugger if it was using a Module, so that we can query for interruption in Module code where we don't have access to a debugger. I used that to support interrupting debug info reading on a per-module basis - since that's a long-running but - at least on a module boundary - resumable operation.

The current method of checking "Does this module one belong to a debugger requesting interruption" is quick and dirty here. IMO, a better solution would be to have Modules keep a list of the Debuggers (or maybe Targets) using them. But we should figure out all the ways we want to use that and design something nice for the purpose.

Diff Detail

Event Timeline

jingham created this revision.Jul 5 2023, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 1:10 PM
jingham requested review of this revision.Jul 5 2023, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 1:10 PM
mib added a comment.Jul 5 2023, 2:09 PM

This looks very cool! I left "a few" comments on how I think we can simplify this patch, and also had some remarks regarding the in_process_target sanity checks.

lldb/include/lldb/Core/Debugger.h
435–436

If you use LLVM_PRETTY_FUNCTION you won't need this.

446

You could use LLVM_PRETTY_FUNCTION which will give you both the function signature and the argument well formatted.

454
460–468

Using LLVM_PRETTY_FUNCTION we can simplify the class by removing the variadic constructors and replace to std::string with a llvm::StringLiteral

lldb/include/lldb/Target/TargetList.h
192

Can the module be null ? If not, can we pass a reference instead of a raw pointer here ?

200

Same question as above, can any of the in_process_target be null ? If not, lets make it a Target&

216

ditto

218

ditto

220

ditto

lldb/source/API/SBFrame.cpp
57–58

Is this still necessary ?

lldb/source/Commands/CommandObjectTarget.cpp
67–68

Is this still necessary ?

lldb/source/Core/Debugger.cpp
1278–1286

You can get rid of this is you use LLVM_PRETTY_FUNCTION.

1288–1292

It would be nice if InterruptionReport had a Dump method.

lldb/source/Core/Module.cpp
1072–1073

You can get the Module reference by dereferencing this.

I really like the change to the interface, I especially like that it can report what was interrupted and how much work actually done. I had a lot of the same feedback as Ismail but also had some questions to help me understand all the details.

lldb/include/lldb/Core/Debugger.h
430–431

I think it would make more sense to have cur_func and formatv be of type llvm::StringRef. Concretely, if you construct a std::string from nullptr (like you're doing below when you make an InterruptionReport object), you will crash.

I know the recommendation is to use INTERRUPT_REQUESTED instead of filling this manually, but inevitably somebody will go against the advice and make a mistake.

446

+1

455–456

To avoid some unnecessary copies

Could also do what Ismail is suggesting.

465

since we're passing format to formatv, I think it would make sense for its type to be llvm::StringRef up-front here.

lldb/include/lldb/Target/TargetList.h
191
192

+1

Alternatively, if these are being pulled out of shared pointers, pass the shared pointer directly so we can avoid the issue of dangling raw pointers if the module ever is freed by the shared pointer.

200

+1

Same as above, let's not circumvent shared pointer semantics since we're storing references/pointers to these objects.

216–221

Same as my comment above.

lldb/source/API/SBFrame.cpp
57

What is this used for?

lldb/source/Commands/CommandObjectTarget.cpp
67–68

Where is this used?

lldb/source/Core/Debugger.cpp
1266–1267

Did clang-format give this to you?

1290–1291
lldb/source/Core/Module.cpp
1076
1078

I don't think you need to specify this->?

lldb/source/Target/StackFrameList.cpp
31

Where is this used?

lldb/source/Target/TargetList.cpp
518

I'm somewhat puzzled by this. Why are we unregistering the target in AddTargetInternal when we're registering it in CreateTargetInternal? Maybe I don't understand the intent behind {Register,Unregister}InProcessTarget.

mib added a comment.Jul 5 2023, 3:06 PM

I had an offline chat with Jim and I misunderstood originally, what he was trying to achieve with the InterruptionReport:: m_function_name so my suggestion of using LLVM_PRETTY_FUNCTION doesn't actually work here.

jingham added inline comments.Jul 5 2023, 3:26 PM
lldb/include/lldb/Core/Debugger.h
435–436

I'm not sure I understand how you want to use LLVM_PRETTY_FUNCTION. From what I can tell, LLVM_PRETTY_FUNCTION prints something that looks like the function invocation as written. But ReportInterruption and the associated macros provide a formatv API.

lldb/source/Target/TargetList.cpp
518

The point behind this is someone says "make me a target" and then in the process of making the target, the Target code does something slow (e.g. look for external debug files) that we want to interrupt. So we need a way for the debugger to find the "in the process of being made" Targets.

"AddTargetInternal" is the API where the target gets added to the Debugger's TargetList, so after that method has run we will find the target in the regular TargetList. But between CreateTargetInternal and AddTargetInternal, that target isn't stored anywhere that the Debugger knows how to query. I added this list to hold the targets that are in the process of getting made, before they get added to the Debugger's TargetList.

jingham updated this revision to Diff 537544.Jul 5 2023, 4:40 PM

Address review comments:

Made the in_process target list shared pointers.

Removed sstream includes.

jingham added inline comments.Jul 5 2023, 5:03 PM
lldb/include/lldb/Core/Debugger.h
430–431

I think it would make more sense to have cur_func and formatv be of type llvm::StringRef. Concretely, if you construct a std::string from nullptr (like you're doing below when you make an InterruptionReport object), you will crash.

I know the recommendation is to use INTERRUPT_REQUESTED instead of filling this manually, but inevitably somebody will go against the advice and make a mistake.

I don't see how I can make the formatv option a StringRef. The llvm::formatv API only offers a version that takes a const char *. Anyway, these are formatv strings, they are almost universally going to be const strings. Turning them into llvm::StringRef's and back out again to use in llvm::formatv seems odd.

But you are right, we should protect against someone passing in a null pointer for the function or format to InterruptRequested. Since this is just logging, an assert seems overkill, I'll just add null pointer checks here and turn them into "UNKNOWN FUNCTION" and "Unknown message".

lldb/source/API/SBFrame.cpp
57–58

Not sure what you are asking here. We use StackFrameSP w/o saying lldb::StackFrameSP and we use VariableList for instance rather than lldb_private::VariableList. We could remove the using statements here but that's not how we do it in general. In some cases we don't do using namespace lldb but that's mostly in files that use clang API's heavily since those conflict with some clang type names and it was good to be explicit there. But I don't think we want to be verbose like that in the SB files.

lldb/source/Target/StackFrameList.cpp
31

Dunno, but this seems more like an orthogonal cleanup unrelated to the current patch.

jingham updated this revision to Diff 537551.Jul 5 2023, 5:09 PM

Protect InterruptRequested from null function & format strings.

lldb/include/lldb/Core/Debugger.h
455–456

This is a local that is copied to an ivar and never used again. Do I really have to put move in there explicitly for the optimizer to know it can reuse the value?

bulbazord added inline comments.Jul 6 2023, 8:42 AM
lldb/include/lldb/Core/Debugger.h
430–431

Oh... so llvm::formatv does take a const char *... My bad there, that makes sense, no need to change the type of format then.

Otherwise, I'm satisfied with the safety checks.

455–456

Yes. I created a simple example on Godbolt: https://godbolt.org/z/G4ej76Enn

Observe that the constructor in the example invokes the std::string copy constructor. Add a std::move and it then invokes the move constructor.

465

As you've pointed out above, formatv takes a const char * so ignore this.

lldb/source/Core/Debugger.cpp
1280

You'll also want to std::move(function_name) here too, I think.

lldb/source/Target/TargetList.cpp
518

Gotcha, that makes sense. Thanks for explaining!

jingham added inline comments.Jul 6 2023, 2:42 PM
lldb/include/lldb/Core/Debugger.h
455–456

Thanks for the example, that was interesting.

Note however that in your example, you the flags you specified were -std=c++17 so that was an unoptimized build. That's expected, unoptimized builds are supposed to be as literal as they can be. If you instead use the same example with -std=c++17 -O3 you won't see any copy constructors. I don't 100% mind putting this std::move in, but it seems noisy to decorate up your code with hints that the optimizer can figure out on its own.

bulbazord added inline comments.Jul 6 2023, 3:10 PM
lldb/include/lldb/Core/Debugger.h
455–456

You're right that I did not consider higher optimization levels there. That being said, I believe this is a matter of language semantics and not optimization level. I've created a slightly more complex example to illustrate: https://godbolt.org/z/naMTcd7Ev

In this other example, I have passed -O3. You'll notice that in the emitted constructor code there are 2 calls to operator new. If you add std::move for both of the parameters of the constructor, both of these allocations go away and the generated code becomes ~60% smaller. Even with optimization involved, the compiler can only optimize so much without breaking semantics.

jingham updated this revision to Diff 537908.EditedJul 6 2023, 3:35 PM

Add some std::move's, Jonas agrees all the cool kids are doing that these days.

bulbazord accepted this revision.Jul 6 2023, 4:13 PM

LGTM!

This revision is now accepted and ready to land.Jul 6 2023, 4:13 PM
This revision was automatically updated to reflect the committed changes.
mib added a comment.Jul 6 2023, 4:20 PM

LGTM! Thanks Jim!

MaskRay added a subscriber: MaskRay.Jul 6 2023, 8:30 PM

Hi! The added lines have some spaces at line ends. I think using clang-format-diff.py would remove them.