Page MenuHomePhabricator

[lldb] Add helper class for dealing with key:value; GDB responses
Changes PlannedPublic

Authored by DavidSpickett on Dec 14 2020, 8:27 AM.

Details

Summary

The current handling of qHostInfo is a bunch of if-else,
one for each key we think we might find. This works fine,
but we end up repeating parsing calls and extending it
gets tricky.

To improve this I'm adding KeyValueExtractorGDBRemote.
This is a class that takes the packet response and splits it up
into the key-value pairs. Where a pair is "<key>:<value>;".

Then you can do Get<T>(<key>) to get an Optional<T>.
This Optional is empty if the key was missing or its value
didn't parse as the T you asked for.

Or you can GetWithDefault<T>(<key>, <default value>)
which always gives you a T but if search/parse fails
then you get <default value>.

Plus specific methods to get LazyBool and hex encoded
string keys.

This improves the packet handling code by:

  • Putting the type, key name and default value on the same line.
  • Reducing the number of repeated parsing calls. (fewer "!value.getAsInteger(0, ...)" means fewer chances to forget a "!")
  • Removing the need for num_keys_decoded, which was essentially a boolean but was treated as a running total. (replaced with HadValidKey)

We do pay an up front cost in building the map of keys up
front. However these packets are infrequent and the increase
in readability makes up for it.

This change adds the class and applies it to qHostInfo, keeping
the original logic intact. (future patches will apply it to
other packets and refactor post decode logic)

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll

Event Timeline

DavidSpickett created this revision.Dec 14 2020, 8:27 AM
DavidSpickett requested review of this revision.Dec 14 2020, 8:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 8:27 AM

I realise it's a lot to review but if you have any initial comments on the usability that'd be great. My main point here was to make the if-else chain a bit "safer" and more easily extended. The parsing logic itself I did not change, to keep things clear (though I would like to improve it later).

I've also put up https://reviews.llvm.org/D93226 where you can see how it changes qProcessInfo parsing.

There are a few more packets that could benefit to varying degrees. (some are just 1/2 keys)

  • qGDBServerVersion
  • qMemoryRegionInfo
  • qWatchpointSupportInfo
  • qProcessInfo again (in GetCurrentProcessInfo)
  • qLaunchGDBServer
  • qModuleInfo

A quick look over them doesn't show anything that would prevent KeyValueExtractorGDBRemote from applying there too if it proves useful here.

I think that streamlining the packet (de)serialization process is badly needing. I'm not so sure about the approach though. On one hand, it's definitely an improvement. On the other, it does not handle the deserialization process, and I was hoping we could have a solution which handles both, for two reasons:

  • it makes things simpler
  • it allows us to avoid the need for tests which explicitly test serialization and deserialization of each packet, as the things is implemented (and tested) once, centrally.

Have you looked at the how llvm YAML and JSON libraries handle (de)serialization? I was hoping that we could implement something similar to that...

DavidSpickett planned changes to this revision.Dec 18 2020, 2:21 AM

Have you looked at the how llvm YAML and JSON libraries handle (de)serialization? I was hoping that we could implement something similar to that...

Good point, I was only looking at half the issue. I'll see how those libraries do things.