- User Since
- Oct 8 2012, 9:19 AM (301 w, 6 d)
Fri, Jul 20
So long as the tests don't fail with the feature enabled, that's probably
Looks good to me (maybe add a negative test case to ensure the full text is not dumped when neither the environment variable or command line parameter is used - I imagine most/many FileCheck test cases might be silently passing even if this behavior was enabled by default?). Also maybe the command line argument version of the test should specifically unset the environment variable - to make sure the test is testing what's intended even in cases where someone is setting the env variable on by default (some users may do this, or buildbots)? (worth checking that all the tests pass even if you turn this option on by default - maybe some FileCheck self-tests may fail when this unexpected output turns up?)
Thu, Jul 19
Probably squirrel it away in the generic 'flags' bucket?
Couple of other possible ideas:
Anyone thought about whether it'd be worth making the indexed V unindexed
case work implicitly? If we were willing to add another pointer to
DwarfStringPoolEntryRef (doubling its size, admittedly) - back to the
string pool itself - then we could index any string where the
DwarfStringPoolEntryRef was asked for an index, and otherwise skip that?
Rather than having to declare up-front whether an index was required.
A more detailed commit message (is this meant to change behavior? what's change? if it's mostly/only a refactor - what's the end goal? (or is this retroactive tidy-up?)) would be good.
Wed, Jul 18
There is future work planned (at my office) to address the interpretation of a sequence of .loc directives with no interleaved instructions.
Looks good - thanks!
Tue, Jul 17
I feel like maybe the solution here should be in the MC integrated
assembler - to not emit zero-length sequences in the line table, regardless
of whether it's in the prologue or anywhere else?
Test coverage (in test/FileCheck) would be great!
Could you provide a small example dump of the invalid line table you're addressing?
Mon, Jul 16
Seems good to me - though I haven't looked too closely/don't know this code terribly well (so you're welcome to wait if you know someone else more knowledgable might take a look too - or if you're fairly confident, commit & they can always follow up in post-commit)
Oh, bother. I tried adding an llvm::reverse call over DWARFDie.children() and it still compiled, even though I made DWARFDie::iterator an input_iterator - so I guess std::reverse_iterator isn't checkingf the trait? :/ That's unfortunate.
@rsmith - Richard, I'd be curious for your opinion here. I've been casting through the runes of the standard for C++ iterator semantics/guarantees, and I can't find where the iterator requirements are that makes std::reverse_iterator's behavior valid (basically relying on the underlying sequence having indefinitely persistent storage - that a pointer/reference to an element is valid even after the iterator that pointer/reference was obtained from goes out of scope). And whether there's some other solution - a common/predefined type trait for saying "hey this isn't valid" or "this is the reverse iterator type for this sequence" (so llvm::reverse(Range) could use that trait to decide how to get reverse iterators from the forward ones) or if we should create such a trait (though I guess it'd still be problematic for any other iterator adapters having to somehow forward through/expose such details... :/)?
Looks good, Thanks!
Ah, yeah, fair point that they're still implementation details/can't be
included from tests - fine fine (:
Might be worth some unit tests now that these are reusable components.
In theory you'd need separate tests for op== and op!= returning false (currently all the tests would pass if both implementations returned true in all cases), but not the biggest deal.
If the implementation of a function requires a string - it should probably
accept string (not a StringRef) as a parameter - to avoid back-and-forth
conversions in cases where the caller already has a string to use.
Any chance this can/should be unit tested? (also, in general (though might
not matter in this instance), symmetric operators like == should be
implemented as non-members (though they can still be friends and if they
are, can be defined inline in the class definition as a member could be),
so any implicit conversions apply equally to the LHS as the RHS of the
Fri, Jul 13
Wed, Jul 11
Tue, Jul 10
So we give back the user an event object that has as wide of a common interface as possible, but still doesn't prevent access to all of the bits. The consumer of the common interface will be common code. Code that wants to decide what to do after hitting a breakpoint probably doesn't need all this platform specific information. It can just stop, look up line and symbol information based on the address, and report it to the user. But in a system like this which has deep hooks into the OS, it will be very frequent that the code handling an event needs to do something completely different depending on the platform. So we branch out into platform specific code, and in that platform specific code we know how to consume the platform specific event data.
That seems plausible (op<< for Error) - but does that handle the case
mentioned above "we may not actually want to produce the string (eg: if the
Logger implementation decides to filter out the formatv_object_base based
on level)" - is op<< still called even with this filtering? (seems like if
it was, then it wouldn't avoid the stringification costs? but if it doesn't
call the op<< then it'll still fail the consume semantic requirement of
Mon, Jul 9
Only lambdas, and not other functors (like std::function, etc)?
I don't happen to have a setup to test this - but if it works for you, I'm fine with it :)
Mon, Jul 2
Which was the first patch that passed the buildbots? Could you
point/mention the commit revision? & then the commit revision that failed?
I hadn't realized a version had been committed before.
Perhaps generalized further still - if every format has the ability to retrieve a section by name or by index and return a generic llvm::object::SectionRef - then the code can be written in general over that, query the SectionRef for the name and contents, etc?
This feature was/is only enabled in -O0, though, right? As a debugging aid,
if I recall correctly - because it can be super confusing when your code
falls off the end of one function into the next. Is the code size savings
(only at O0) worth the increased cost to debug?
Hey Zach - what's your initial use case/motivation for this?
Out of curiosity, what's the use case/motivation for this?
Fri, Jun 29
Probably worth generalizing over the other formats too (again, refactoring as much as possible into common helper functions - guessing most of the contents of "printSectionAsString" could be generalized over all the different formats without duplicating it in each one. But that can be done in follow-up patches.
Yeah, doesn't look like this code handles a value crossing the boundary of
the size of the bitfield type (it's reading only the low bit).
Thu, Jun 28
Side note: please don't depend on what type a pointer points to, if
possible (& it should be possible everywhere except where necessary to
match the IR constraints - eg: you may need to check a pointer's pointee
type to ensure a use of a value of that type matches up with its
definition). Eventually pointee types will go away & someone will have to
fix up that code... best to avoid that if we can.
Wed, Jun 27
Add the inverse equality test too, perhaps - EXPECT_EQ(V1, AR1) ? Though I realize that looks uninteresting & it is, apparently - that's the tricky case for when op overloads are members (though this change doesn't make it a member)
Tue, Jun 26
Looks pretty good to me - nice work!
Looks good - thanks!
I think we now have adl_begin/adl_end helpers in the llvm namespace that
should do the right thigh (which is that the call should be made like
"using std::begin; ...begin(X)..." The same way swap is meant to be called
(this allows a default implementation in the std namespace and allows for
types to implement their own custom implementation in their own namespace
to be found via ADL))
Mon, Jun 25
Nah - I was thinking of something where zip wouldn't need any changes,
potentially. One possibility:
This doesn't seem to build for me - so hard to debug/probe it:
Idle thought: Could/should this be composed from other primitives that could be created. Such as an infinitely long (or maybe bounded, not sure) range built from another range and a functor of some kind? (or such a thing specifically tailored for Optional-or-default-T situations) & then zip two of those together & bound that resulting infinitely long range (or bound the input ranges, either way).
Still not quite sure why the change in implementation of Optional - what the difference is between how it was done & how it's done now/with this patch, etc. But I'm not too fussed - if it works for you :)
Thanks for adding to/improving the GDB pretty printer support! Just a few questions/thoughts to try to understand how this works similarly/differently from other related printers!
Jun 21 2018
*looks at this a bit more closely*
Yeah, I know nothing about this dump feature or what's being fixed here - test cases would be great to help motivate/explain.
Jun 20 2018
Could/should this be format-agnostic? (looks like this change only implements the functionality for ELF - but I'd imagine the existence of sections and their contents might be general enough to allow for different object file formats to support this without each format having custom handling)
Jun 18 2018
Could you add a test case?
Could you add a test case?
Jun 15 2018
Not sure I should get too involved in this discussion, knowing so little about Objective C - but if it seems sensible, could you provide some examples (just in comments/description in this code review) of what the DWARF used to look like and what it looks like after this change?
Feel free to commit changes like this without review - seems sensible/obvious enough. (in theory you could write a unit test, perhaps? but hardly seems worth it for this change - maybe if an existing test covers this code you could change it to have a const parameter)
Jun 14 2018
Probably overkill, but I'd imagine the way to do this would be a trait
class that implements the two different tests depending on the type of the
node. (I guess a generic "isDebugInstruction" function that uses that trait
could be a handy general purpose tool to have, maybe? (so you don't have to
think so hard about different way s of testing whether something is a debug
instruction in MC versus IR))
gah, sorry - failed to submit my comment.
Jun 12 2018
ah, yeah - thanks for pointing out/explaining the existing tests a bit & how they're re-executing the full gtest then waiting to be run themselves, etc.
Jun 11 2018
Seems good to me
Jun 8 2018
Seems OK to me (:
Thanks for adding some test coverage! Few things that might be able to be simplified before committing.
Jun 6 2018
Seems OK to me - but not necessarily a strong motivation. Is the SmallPtr header dependency a significant impact? Would it be easier for to go the other way & normalize/standardize on SmallSet everywhere, if it reasonably does the right thing for pointers?