- User Since
- Mar 2 2013, 8:12 AM (369 w, 3 d)
CC'ing the DWARF cabal.
For the future, a clean solution would be extending the macros in Dwarf.def to list the stack effects in the definitions of the DW_OP_*, for example
This is obviously good! Do you think that a similar error handling bug might exist in other cases that depend top-of-stack?
In case of Apple platforms, this won't make a difference in practice, since the support for that is implemented in PlatformDarwin (which all of these inherit from), but it sounds like this will be a problem for the "linux" sdk (assuming this is what I think it is), as there the selected platform will be PlatformLinux, which has no clue about these sdk thingies.
Fri, Mar 27
I've reworked this a little based on your feedback.
Remove the obsolete setter in lldb::Type.
Rebase and update based on https://reviews.llvm.org/D75488. It no longer takes a reference, but is a value type instead.
LLDB didn't apply DW_AT_comp_dir to DW_AT_LLVM_include_path. I fixed that now.
Thu, Mar 26
I don't really know enough about this to give a meaningful review.
LGTM but since we're on the same team (now, woot!) I'd like to give others a chance to have a look.
Wed, Mar 25
Couple of minor suggestions inside.
I added a comment to D75488. It's in getOrCreateModule().
I only have a bunch of superficial comments but this seems reasonable as far as I can tell.
Tue, Mar 24
I'll update this with either a fix in clang or a patch to lldb once I figured out what happened.
I had to revert this because it unexpectedly broke the "expr -- @import Module" test in LLDB.
Mon, Mar 23
In your example you're encoding set as a derived type over an enum. That seems like a fine approach to me, but I think the interface should enforce this. Perhaps the Verifier should reject set derived typed from anything but an enum and the DIBuilder should only accept DICompositeTypes as the base type? Are there non-enum set types that you also want to encode?
Does this also need dwarfdump support, or does this work already?
Fri, Mar 20
Yeah, screwed up during rebasing and accidentally committed an extra file from a different review I reverted and relanded the patch now and the bots are running again. Sorry for the noise.
Since it's late on Friday afternoon I'm taking the liberty to revert the patch to get the bots running again. Feel free to re-land with a fix or ask for another round of review if you have more questions.
It looks like this test doesn't work on the bots: http://green.lab.llvm.org/green//view/LLDB/job/lldb-cmake/lastCompletedBuild//testReport/junit/lldb-api/python_api_sbenvironment/TestSBEnvironment_py/
Looks like a great start! (From my side)
Rebased and addressed review comments.
Thanks, Pavel, these are all very valid concerns and in retrospect I should have at least slapped an RFC label on this before dumping into phabricator last night. The goal of this (and I should have mentioned that in the description) is to make the Xcode SDK something that behaves more like the compiler's resource directory, as in that it ships with LLDB rather than with the debugged program. This important primarily for importing Swift and Clang modules in the expression evaluator, and getting at the APINotes from the SDK in Swift.
Should we then call it SmallCoalescingBitVector? Probably not :-)
Thu, Mar 19
Don't try to be too clever with the strings.
Don't try to be too clever.
Wed, Mar 18
Rolled https://reviews.llvm.org/D75626 back into this patch, addressed more review feedback.
And then there's ClangModulesDeclVendor::ModuleID -- I have no idea what's the relationship of this to that..
I'm merging this back into https://reviews.llvm.org/D75488 for easier reviewing.
I think this is starting to look good now. I found two more opportunities for improving the output... sorry about finding these piecemeal.
You could add a unit test for this, but it would effectively be a reimplementation of the function. I'm fine either way.