This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix JSON parser to allow empty arrays
ClosedPublic

Authored by tetsuo-cpp on Sep 27 2019, 7:34 PM.

Details

Summary

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=39405

alexc@kitty:~/work/wiredtiger/build_posix$ cat breakpoint.json
[{"Breakpoint" : {"BKPTOptions" : {"AutoContinue" : false,"ConditionText" : "","EnabledState" : true,"IgnoreCount" : 0,"OneShotState" : false},"BKPTResolver" : {"Options" : {"NameMask" : [56],"Offset" : 0,"SkipPrologue" : true,"SymbolNames" : ["__wt_btcur_search"]},"Type" : "SymbolName"},"Hardware" : false,"SearchFilter" : {"Options" : {},"Type" : "Unconstrained","Foo" : []}}}]

Before

(lldb) breakpoint read --file breakpoint.json
error: Invalid JSON from input file: /home/alexc/work/wiredtiger/build_posix/breakpoint.json.

After

(lldb) breakpoint read --file breakpoint.json
New breakpoints:
Breakpoint 1: where = libwiredtiger-3.2.2.so`__wt_btcur_search + 15 at bt_cursor.c:522:5, address = 0x00007ffff576ab2f

Diff Detail

Event Timeline

tetsuo-cpp created this revision.Sep 27 2019, 7:34 PM
davide requested changes to this revision.Sep 27 2019, 10:05 PM
davide added a subscriber: davide.

This needs a test. You can either write a python one as the ones in test/testcases or a lit-style based as the ones in lit/.

This revision now requires changes to proceed.Sep 27 2019, 10:05 PM
labath added a subscriber: labath.Sep 30 2019, 12:06 AM

This needs a test. You can either write a python one as the ones in test/testcases or a lit-style based as the ones in lit/.

Another option might be a c++ unit test for the JSON class (see unittests/Utility/JSONTest.cpp).

It would be good as Pavel and Davide suggest to write a test directly for the JSON parser. Doing so in the C++ Unit test seems the most convenient, but that's up to you. But it would also be good to add a test for the particular "breakpoint write" -> "breakpoint read" scenario that uncovered this bug. There are already some tests for that in the functionalities/breakpoints/serialize test case, so it should be easy to add one there.

Hey, I just want to give you a heads up that I'm in the process to replace LLDB's JSON implementation with the one from LLVM. The parts in StructuredData are already gone (r373359, r373360) and I'm currently working on the other uses in LLDB, except for debugserver which I'm not planning to touch for now.

Hey, I just want to give you a heads up that I'm in the process to replace LLDB's JSON implementation with the one from LLVM. The parts in StructuredData are already gone (r373359, r373360) and I'm currently working on the other uses in LLDB, except for debugserver which I'm not planning to touch for now.

Thanks for letting me know.
Did you want the debugserver portion of this change? Or should I just close this.

tetsuo-cpp updated this revision to Diff 222923.Oct 2 2019, 3:47 PM

Rebased onto trunk.

Thank you for the suggestions!

I'm assuming that since only the debugserver portion of this change is left, I should write a test specifically for that. I had a quick look and it wasn't obvious how to do that but I will spend some more time on it this weekend.

@jingham
Regarding the idea of a serialization test, I've only ever triggered the bug by manually adding an empty array to some breakpoint JSON. I'm not sure how the reporter stumbled across the issue "naturally". Is it ok to have a serialization test where the test itself edits the JSON after it has been written? Or would it be better if I find some scenario where LLDB writes JSON with an empty array and trigger the bug by reading it back in?

tetsuo-cpp updated this revision to Diff 228561.Nov 9 2019, 1:03 AM

I've had a try at writing some unit tests for the JSON parser in debugserver, including the empty array case which I'm fixing in this patch.

Friendly ping. Could I please get this looked at?

davide accepted this revision.Nov 13 2019, 9:22 PM

Thanks for adding the tests. I took a look at the patch and it seems fine to me. Do you need somebody to commit this for you?
@jingham or @labath may want to have another look.

This revision is now accepted and ready to land.Nov 13 2019, 9:22 PM

Looks good to me. If I was writing the test, I'd probably just make it do a string->json->string round-trip and then verify the final string. But this is fine too...

lldb/unittests/debugserver/JSONTest.cpp
17–18

You may want to change these into ASSERT_xxx to avoid crashing later if the expectations are not met.

tetsuo-cpp marked an inline comment as done.Nov 14 2019, 5:58 AM

Thanks for looking at this. I will need someone to commit it for me.
However, I've been having issues with the test suite on my MacBook. check-lldb-unit works for me but check-llvm and check-lldb are hitting issues because of something wrong with my environment. So I'll need to sort this out first to verify that I haven't broken anything before it's committed. I'm hoping to spend some time this weekend to debug my setup.

Thanks for looking at this. I will need someone to commit it for me.
However, I've been having issues with the test suite on my MacBook. check-lldb-unit works for me but check-llvm and check-lldb are hitting issues because of something wrong with my environment. So I'll need to sort this out first to verify that I haven't broken anything before it's committed. I'm hoping to spend some time this weekend to debug my setup.

Once this is approved, I can run the tests for you to make sure it didn't regress [I do that regardless before committing].
That said, it would be good if you got your environment set up correctly [if there's something you don't understand, feel free to reach out on llvm-dev or lldb-dev, depending on the question].

labath accepted this revision.Nov 15 2019, 4:10 AM

I'm not sure whose approval are you waiting for, but this LGTM.

Thanks for the reviews!
@davide could you please commit this on my behalf?

Thanks for the advice. I intend to continue working on LLDB when I can so I will spend some time to fix the test suite on my Mac.

This revision was automatically updated to reflect the committed changes.