Page MenuHomePhabricator

[Bug 49018][lldb] Fix incorrect help text for 'memory write' command
ClosedPublic

Authored by RamNalamothu on Tue, Nov 23, 9:03 AM.

Details

Summary

Certain commands like 'memory write', 'register read' etc all use
the OptionGroupFormat options but the help usage text for those
options is not customized to those commands.

One such example is:

  (lldb) help memory read
           -s <byte-size> ( --size <byte-size> )
               The size in bytes to use when displaying with the selected format.
  (lldb) help memory write
	   -s <byte-size> ( --size <byte-size> )
               The size in bytes to use when displaying with the selected format.

This patch allows such commands to overwrite the help text for the options
in the OptionGroupFormat group as needed and fixes help text of memory write.

llvm.org/pr49018.

Diff Detail

Event Timeline

RamNalamothu requested review of this revision.Tue, Nov 23, 9:03 AM
RamNalamothu created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 23, 9:03 AM
RamNalamothu edited the summary of this revision. (Show Details)Tue, Nov 23, 9:16 AM

Thanks for looking at this bug! I admit I don't know the option parsing very well which is why I shied away from fixing it myself.

Some good finds elsewhere too, but better as their own changes I think. (feel free to add me on review there too)

lldb/include/lldb/Interpreter/OptionGroupFormat.h
37

Would be good to add a comment here, something like:

// The default help text is written for "memory read", this can be used to override that
84

If the message is a const char* do we need to store this in a std::string? I'm, perhaps naively, assuming that you'll only do this override once. Or that if you did it again in a subclassed command, it would replace the whole text.

lldb/source/Commands/CommandObjectMemory.cpp
1177

Can you do this like this? Saves writing push_back a few times.

return { std::make_tuple<...>, ... };

Or you could construct this directly in the constructor parameters below. I'm not sure you need a new function for it.
(depends what clang-format makes of it I guess, if it's unreadable then sure have a utility function)

1251

Which part of this change does this contribute to?

1295

Great job spotting this, but I'd rather it was in its own change with its own test.

lldb/source/Interpreter/OptionGroupFormat.cpp
73

Not sure if clang-format has done this, or just your preferred style. Nothing against either but it makes it difficult to tell if anything has changed in this particular part of the diff.

In general we want clang-formatted code but if it makes the diff tricky to read it's best done in a follow up change that just does formatting.

80

As I said above, if this is about commands being able to *replace* the help text I don't think append is needed here.

Unless std::string is helping you check whether it's been set or not, which I think you could do with nullptr just as well but please correct me if I'm missing some context.

lldb/test/API/commands/help/TestHelp.py
246

If these memory write <options> lines are checking for the other issue you found, I'd put those in their own test.

Not sure if you're a regular contributor and already know this but in case not, I tend to use https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting for doing clang-format stuff. It'll format only the lines you've changed.

Not sure if you're a regular contributor and already know this but in case not, I tend to use https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting for doing clang-format stuff. It'll format only the lines you've changed.

Thanks for sharing that. I normally use git-clang-format (i.e. clang-format extension for git) which does the same job.

lldb/source/Interpreter/OptionGroupFormat.cpp
73

The clang-format did it. Will mark this with clang-format off/on.

80

I kind of took the std::string approach from OptionGroupPythonClassWithDict but as you mentioned it is not needed and will fix that.

RamNalamothu edited the summary of this revision. (Show Details)

Address review comments.

RamNalamothu marked 8 inline comments as done.Thu, Nov 25, 2:53 PM

Remove the unnecessary if {} gaurding around range for loop.

Thanks for splitting the patches.

lldb/source/Interpreter/OptionGroupFormat.cpp
23

You could use a std::copy for this and not need to spell out each index.

73

My bad, what I meant was that it's fine to format it in general but that in this case it should be avoided to make the diff more readable.

But now I see that this is essentially a new function so formatting it all is fine. I now see the change you're making, the body of the array/function isn't actually changing. You can remove the /* clang-format...*/ and just let it do its thing.

That said, what is the reason that you couldn't re-use the option table?

I have two thoughts about the function:

  1. Do we need to remake the array each time, vs the constexpr array from before.
  2. We're returning an ArrayRef to a return value, which probably works in some situations (if you immediately do a copy using the ref, then discard it) but I think it's a lifetime issue overall.

I'm assuming that ArrayRef is basically std::array& in a general sense, and if you returned a std::array& to something on the stack, that's a lifetime issue for sure. I looked at some other option groups and they all follow the pattern of returning an array ref to a const array defined outside the function.

RamNalamothu added inline comments.Fri, Nov 26, 4:58 AM
lldb/source/Interpreter/OptionGroupFormat.cpp
73

Ah, I could re-use the option table. Just an oversite from my end. Thank you for spotting that.

Re-use the existing constexpr option table.

This revision is now accepted and ready to land.Fri, Nov 26, 5:17 AM