- User Since
- Jun 4 2013, 6:02 AM (267 w, 2 d)
The Stream part isn't the problem. The problem is that the dumping code is implemented in terms of ExecutionContextScope, which then pulls in pretty much everything. If it was just the object "dumping itself" then I would be fine with it as a method, but here it's using the whole world to achieve that goal. At that point I would prefer it living in a separate place (though we can debate on whether it should be a completely separate file, or merged to a single file with the DumpDataExtractor code)
Does anyone have an opinion on this?
This technically isn't NFC, because it changes the StringPool used for
apple tables in the DWO case (now it uses the main file like DWARF v5
instead of the DWO file). However, that shouldn't matter, as DWO is not
a thing on apple targets (clang frontend simply ignores -gsplit-dwarf).
Fix typos and add the extra test.
I am not sure what kinds of problems this can cause, but I can volunteer to help with the lldb side of things in case of any fallout.
This was reverted in r333482 because it was causing "definition with same mangled name as another definition" errors in some internal google builds.
Tue, Jul 17
Updated the test to include the two extra strings that are used from the
I suppose we can add an off-by-default DWP mode so that it can be used for integration testing.
The tests seem fine, but as a matter of style I would suggest two changes:
- replace ASSERT_XXX with EXPECT_XXX: EXPECT macros will not terminate the test, so you'll be able to see additional failures, which is helpful in pinpointing the problem. ASSERT is good for cases where the subsequent checks make no sense or will crash if the previous check is not satisfied (but that is not the case for your checks AFAICT).
- replace ASSERT_TRUE(foo == bar) with ASSERT_EQ(foo, bar) (and similar for other relational operators). Right not that does not matter much, but if someone later implements operator<< for this class, you will automatically get a helpful error message describing the objects instead of a useless "false is not true" message.
Normally this would be clearly a good thing, but the added complication here is that this function is part of a class hierarchy, and so this way you are forcing every implementation to take a std::string, even though only one of them cares about null-termination.
I have no opinion on the implementation, but I like that you wrote a non-execution test for this, as this will enable us one day to run it on non-windows hosts too.
Mon, Jul 16
Are you worried about the case where the dSYM bundle contains CUs with different dwarf versions?
This looks straight-forward enough to me, though I would feel more comfortable if someone else (@mgorny ?) took a look at this too, to make sure I am not missing anything.
I don't agree with the two-stage initialization of the MinidumpParser class being introduced here. We deliberately introduced the Create static function to avoid this. If this Initialize function in checking invariants which are assumed to be hold by other parser methods, then it should be done by the Create function. Ideally this would be done before even constructing the parser object, but if this is impractical for some reason then you can make the Initialize function private and call it directly from Create. This way a user will never be able to see an malformed parser object. To make sure you propagate the error, you can change the return type of Create to llvm::Expected<MinidumpParser (the only reason we did not do this back then was that there was no precedent for using Expected in LLDB, but this is no longer the case).
Makes sense to me.
Could you please add a test for the new behavior as well?
I think this makes perfect sense. Could you also add the same thing to the other binaries in the tools subfolder?
I think this is fine (though the meaning of SKIP_LLDB_SERVER is subtly different than SKIP_DEBUGSERVER), but while looking at this I got an idea for a possible improvement.
We're also trying to avoid adding new clang-specific code to the debugger core. I think it would make more sense if the (clang-based) c++ highlighter was provided by some plugin. I see a couple of options:
- the c++ language plugin: I think this is the most low-level plugin that is still language specific. However, it is specific to c(++), whereas here you format other languages too.
- the clang expression parser plugin: it's a bit of a stretch, but syntax higlighting is a kind of expression parsing
- a completely new plugin
This looks straight-forward enough.
Could you also add a test case for this?
I think it should be possible to test this via the gdb-client (test/testcases/functionalities/gdb_remote_client/) test suite. If I understood the previous comments correctly, you'll need to mock a server that sends a thread-pcs field, but does not implement a jThreadsInfo packet.
This looks good to me. There are some different tradeoffs we could make (speed/templates vs. code size/virtualization), but I don't have an opinion on that, so if noone else does either, I guess this is fine.
Tue, Jul 10
Sorry for the slow response, I am OOO officially :P.
Mon, Jul 9
Could you elaborate on how/why is this wrong? E.g. in which situations does it matter whether we clear the PC list?
Besides the problems I mention in inline comments, the other issue I have with this patch is that it does things completely different than how we implement the check for libc++, which is an identical problem. (libc++ detection is done by marking all libc++ tests as with a special category, and then deciding in dotest.py (checkLibcxxSupport) whether to run the category as a whole).
Mon, Jul 2
Thank you for working on this. Overall, I like the direction this is going, but I'm OOO this week and the next one, so I'll defer to other reviewers on this one.
Fri, Jun 29
Thu, Jun 28
Add a check to the tab-completion function.
I've had to revert this because it causes compilation errors on MSVC. I expected this could be a problem, though the error itself is rather puzzling:
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\xutility(2919): error C2893: Failed to specialize function template 'unknown-type std::equal_to<void>::operator ()(_Ty1 &&,_Ty2 &&) const' C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\xutility(2919): note: With the following template arguments: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\xutility(2919): note: '_Ty1=const llvm::MDOperand &' C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\xutility(2919): note: '_Ty2=const llvm::MDOperand &'
So it MSVC is picking up this friend declaration (or at least, getting confused by it) even when neither of the types are ArrayRefs. That seems like a bug to me, though we may not be able to do anything about it.
Looks good. Thank you.
Wed, Jun 27
looks good to me
This looks fine to me, but let's also wait for Aaron.
How are you planning on testing this? Most of the DWARFIndex functionality is tested in terms of lldb-test, but I think this function is not reachable from there. Maybe you could write a specific dotest-test which targets this?
Tue, Jun 26
Mon, Jun 25
I suppose this is fine. My main worry is whether llvm-gcc is the *only* situation where we rely on these tweaks, but I guess the best way to find out is to try removing them.
This has broken the LLDB bot http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/25132. Could you take a look?
- Use static factory functions instead of the extra argument (the best names I could come up with is fromData and fromOptionalData).
Fri, Jun 22
Delete copy constructor.
I think I noticed one more bug in the !threads implementation. Other than that, this looks fine to me, but I'll leave the final ok to Chandler.
I think this would be a very nice feature for lldb. Thank you for working on this.