Page MenuHomePhabricator

Add functionality to export settings

Authored by JDevlieghere on Sep 28 2018, 7:57 AM.



For the reproducer feature I need to be able to export and import the current LLDB configuration. To realize this I've extended the existing functionality to print settings. With the help of a new formatting option, we can now write the settings and their values to a file structured as regular commands.

Concretely the functionality works as follows:

(lldb) settings export /path/to/file

This file contains a bunch of settings set commands, followed by the setting's name and value.

settings set use-external-editor false
settings set use-color true
settings set auto-one-line-summaries true
settings set auto-indent true

Loading the settings again is as simple as sourcing the newly created file:

(lldb) command source /path/to/file

I had to make a few small changes here to make this work:

  • When a value is not set settings set <setting> is equal to settings clear <setting>. This is because not all settings have a meaningful default For example for settings set we used to print unknown which is not a value the user can set.
  • I introduced a new printing option and group for printing options as commands. This relates to printing everything on a single line for things like arrays and dicts.

Diff Detail


Event Timeline

JDevlieghere created this revision.Sep 28 2018, 7:57 AM
JDevlieghere edited the summary of this revision. (Show Details)
  • Rebase
teemperor requested changes to this revision.Oct 1 2018, 11:13 AM

Runnings settings export crashes LLDB:

(lldb) down
❲ 8❳ CommandObjectSettingsExport::DoExecute(this=0x0000555555652a80, args=0x00007fffffffd6b8, result=0x00007fffffffdd80) at CommandObjectSettings.cpp:356 ❮❯
   353      result.SetStatus(eReturnStatusSuccessFinishResult);
   355      // Open file for dumping the exported settings.
-> 356      const std::string export_path = args.GetArgumentAtIndex(0);
   357      const uint32_t options = File::eOpenOptionWrite |
   358                               File::eOpenOptionTruncate |
   359                               File::eOpenOptionCanCreate;
(lldb) bt
* thread #1, name = 'lldb', stop reason = signal SIGABRT
    ❲ 0❳ __GI_raise ❮❯
    ❲ 1❳ __GI_abort ❮❯
    ❲ 2❳ __gnu_cxx::__verbose_terminate_handler() (.cold.1) at ❮❯
    ❲ 3❳ __cxxabiv1::__terminate(void (*)()) at ❮❯
    ❲ 4❳ std::terminate() at ❮❯
    ❲ 5❳ __cxxabiv1::__cxa_throw(obj=<unavailable>, tinfo=<unavailable>, dest=<unavailable>)(void *)) at ❮❯
    ❲ 6❳ std::__throw_logic_error(char const*) (.cold.0) at ❮❯
    ❲ 7❳ void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>(this="8����, __beg=0x0000000000000000, __end=<unavailable>, (null)=forward_iterator_tag @ 0x00007fffffffd3f0) at basic_string.tcc:212 ❮❯
  * ❲ 8❳ CommandObjectSettingsExport::DoExecute(this=0x0000555555652a80, args=0x00007fffffffd6b8, result=0x00007fffffffdd80) at CommandObjectSettings.cpp:356 ❮❯
    ❲ 9❳ lldb_private::CommandObjectParsed::Execute(this=0x0000555555652a80, args_string="", result=0x00007fffffffdd80) at CommandObject.cpp:978 ❮❯
    ❲10❳ lldb_private::CommandInterpreter::HandleCommand(this=0x000055555562be30, command_line="settings export", lazy_add_to_history=eLazyBoolCalculate, result=0x00007fffffffdd80, override_context=0x0000000000000000, repeat_on_empty_command=true, no_context_switching=false) at CommandInterpreter.cpp:1702 ❮❯
    ❲11❳ lldb_private::CommandInterpreter::IOHandlerInputComplete(this=0x000055555562be30, io_handler=0x0000555555743b60, line="X����) at CommandInterpreter.cpp:2711 ❮❯
    ❲12❳ lldb_private::IOHandlerEditline::Run(this=0x0000555555743b60) at IOHandler.cpp:562 ❮❯
    ❲13❳ lldb_private::Debugger::ExecuteIOHandlers(this=0x000055555562a620) at Debugger.cpp:975 ❮❯
    ❲14❳ lldb_private::CommandInterpreter::RunCommandInterpreter(this=0x000055555562be30, auto_handle_events=true, spawn_thread=false, options=0x00007fffffffe080) at CommandInterpreter.cpp:2911 ❮❯
    ❲15❳ lldb::SBDebugger::RunCommandInterpreter(this=0x00007fffffffe4c0, auto_handle_events=true, spawn_thread=false) at SBDebugger.cpp:928 ❮❯
    ❲16❳ Driver::MainLoop(this=0x00007fffffffe4a0) at Driver.cpp:1139 ❮lldb❯
    ❲17❳ main(argc=1, argv=0x00007fffffffe6f8) at Driver.cpp:1244 ❮lldb❯
    ❲18❳ __libc_start_main ❮❯
    ❲19❳ _start ❮lldb❯
This revision now requires changes to proceed.Oct 1 2018, 11:13 AM
  • Running settings export /home/teemperor/foobar1 /home/teemperor/foobar2 will actually just create foobar1, but will not report any error that foobar2 is silently ignored.
  • Do we actually have a way to have file completion for the argument? (This doesn't have to be addressed in this review).
  • Using tilde in the export path doesn't work. E.g. settings export ~/foo doesn't create a foo file for me on Linux.

To Raphael's points:

Argument completion handlers are set by the command object implementing HandleArgumentCompletion and dispatching to the CommandCompletions::eDiskFileCompletion. An example of this is in CommandObjectProcessLaunch. Note, arguments currently have a kind (as do options) but for historical reasons, those kinds don't automatically bind to completers. Be nice to finish that bit of the design at some point... But till then, this way though boilerplate is straightforward.

The StreamFile constructor doesn't handle ~. Maybe it should? Or you could make a FileSpec, they do handle ~ completion, and there's a StreamFile constructor that takes a FileSpec.

Breakpoints use "breakpoint write" and "breakpoint read" to save themselves. Maybe it would be clearer if we use the same word for saving breakpoints and saving settings?

I also wonder if it would be good to add a "settings {import, read?}" command to go along with the settings {export,write}. It is a little odd to do "settings export" to export the settings, and "command source" to read them back in. That's relying on the accident that you are exporting the settings as commands, which (a) would be better not to tie ourselves to and (b) users shouldn't have to know that...

It also might be nice to have the output file be a -f option and then I could do:

(lldb) settings export -f foo.cmds

to export all settings and:

(lldb) settings export -f foo.cmds target

to export all the target commands, etc... That might be more flexible for ordinary use. If you do that, the easiest way to specify a completer is by using OptionGroupFile.

213 ↗(On Diff #167722)

"settings clear" also clears the settings value. I'm not sure I'm in favor of having two ways of doing this, especially when the second one is undocumented. Presumably this isn't intrinsic to exporting settings, so it should be done as a separate patch anyway.

331 ↗(On Diff #167722)

Shouldn't this be eArgRepeatPlain? You don't do anything in the case where there's no filename argument.

BTW, I don't think these argument repeat counts are actually enforced. They are used to write out the help strings, but not to check incoming arguments for commands so far as I can see. The development of these and the argument kind specifications stalled when the person who originally worked on them left, and is waiting for somebody to pick them back up again.

356 ↗(On Diff #167722)

You don't know that you actually got an argument. You should check that here.

Thanks a lot for the feedback! I clearly overlooked some stuff when inspiring myself on the other CommandObject code.

I've updated the diff:

  • Rename functionality to read and write for consistency with breakpoints.
  • Add settings read instead of having to source the exported file.
  • Deal with zero or multiple arguments. (This is now valid input)
  • Fix completion for file path.
  • Go through FileSpec to ensure ~ is expanded.
  • Add more tests.
JDevlieghere marked an inline comment as done.Oct 2 2018, 2:00 AM

Only clear setting when the force flag is set (as suggested offline by @jingham).

  • Back to the right diff.
  • Use -f as per the other patch.

@teemperor Can you have another look?

teemperor requested changes to this revision.Oct 24 2018, 4:23 PM

Sorry, this somehow didn't show up in my review queue. I think this can land after two minor things are fixed:

  • I think unknown arguments to write/read shouldn't be silently ignored.
  • And I think we should report an error if writing to the output file fails.

To give a practical example: This command below fails to write to the output file but also doesn't produce any error message.

settings write -f ~/ foo
Note that there is a space behind the /, so we write to a directory (which fails) and we silently ignore the second unknown arg.

This revision now requires changes to proceed.Oct 24 2018, 4:23 PM
teemperor accepted this revision.Oct 24 2018, 4:23 PM

Woops, I wanted to accept in my previous comment.

This revision is now accepted and ready to land.Oct 24 2018, 4:23 PM

Thanks Raphael!

With r345207 the empty behavior only works if you explicitly specify -force. I'll address the second issue before landing this.

Closed by commit rL345346: Add functionality to export settings (authored by JDevlieghere, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.