So long as the tests don't fail with the feature enabled, that's probably
good enough.

In D49328#1170354, @george.karpenkov wrote:

@dblaikie One thing where we could get unexpected behavior is when the environment variable is enabled, but the test does "not FileCheck",
thereby reversing the expected output.
However, I have only seen that done in test/MC/Mips, not in Clang at all, and arguably it's a bad design pattern with double negation,
where using -NOT directives was a way to go.

In D49328#1170336, @george.karpenkov wrote:

@dblaikie done&done. Using =false or =0 is sufficient to get the behavior you describe.

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?)

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.

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!

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?

In D49411#1164680, @labath wrote:

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.

In performance-critical code, llvm would use llvm::Twine as an argument, which is able to avoid copies if the input string happens to be null-terminated (toNullTerminatedStringRef). However, this code is hardly that critical (and ScriptInterpreterPython is the only non-trivial class in the hierarchy), so I don't think it really matters what you do here.

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.

In D49234#1164550, @efriedma wrote:

[forward.iterators]: "Two dereferenceable iterators a and b of type X offer the multi-pass guarantee if [...] the expression (void)++X(a), *a is equivalent to the expression *a." [reverse.iter.requirements]: "The template parameter Iterator shall meet all the requirements of a Bidirectional Iterator". So it's UB to use std::reverse_iterator with your iterator.

There isn't any standard way to handle this situation; libcxx has an internal hack for standard library types (https://reviews.llvm.org/rL300164).

@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
expression)

OK

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
llvm::Error)

In D48807#1150245, @zturner wrote:

Repeating my previous reply since it was through email and didn't make it onto Phab.

In D48807#1150029, @dblaikie wrote:

In D48807#1149775, @zturner wrote:

I've wanted this for quite a long time. My original motivation for this was back when i was working primarily in LLDB. There's a lot of places where void* is passed around as a generic parameter to a user callback. So, for example, you say DoSomethingAsynchronously(Callback, Param) and when the thing is finished, it invokes Callback(Param). There are obviously better ways to do this if you're designing this from the beginning, but it's not always easy to retrofit that into an existing design. So I think several places in LLDB could be cleaned up by replacing some void* with Any. Note those places still exist today, so Any could be useful there right now.

Would it be expensive to reimplement those APIs using stateful functors while providing the backwards compatibility with something like:

void register(void (*f)(void*), void *v) { register([f, v] { f(v); }); }

Or is there something in the implementation (rather than the existing usage) that makes migrating difficult?

It’s been a while since I was close to any of this code so it’s hard to say

More recently while designing a basic API for a tracing library, I've gravitated towards a design where the library generates events (represented by some structure) depending on what happens. But since tracing is heavily platform dependent, there's only so much of this that you can abstract away into a common structure and there will be some additional pieces that only make sense on the particular platform. So you call:

unique_ptr<EventBase> Event = llvm::trace::WaitForEvent();
// Do something depending on what kind of event it is.
llvm::trace::ContinueFromEvent(std::move(Event));

In order for every possible platform to implement this correctly, there may need to be some platform-specific state maintained between the call to Wait and the call to Continue. Linux might need to pass the exact signal value and parameters. Windows might need to pass the exact EXCEPTION_RECORD structure that the OS filled out for us right before we return from WaitForEvent(). It's useful to be able to put that into an Any, so that we have something like:

class EventBase {
  TraceEventType Type;
  uint64_t ThreadId;
  uint64_t ProcessId;
  Any PlatformPiece;
};

class ThreadCreatedEvent : public EventBase {
  void *EntryPoint;
  void *TlsArea;
  // Etc.
};

class BreakpointEvent : public EventBase {
  uint64_t Address;
};

and so forth and so on. When creating this struct before returning from WaitForEvent(), the library can set PlatformPiece to some structure that would contain any platform-specific state needed to properly continue. I prefer not to use inheritance and llvm::cast here (e.g. WindowsBreakpointEvent, PosixBreakpointEvent, etc) because it just leads to inheritance hierarchy explosion and ultimately it's no more type-safe than any (someone still has to make an assumption about exactly what type it is).

If this is host-platform specific, why would this need to be runtime variable at all? Wouldn't the PlatformPiece be hardcoded to some specific layout based on the host platform the compiler was built for/running on?

Not necessarily. For example you may have an x64 build of llvm tracing a 32 bit process.

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 :)

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?

In D48271#1149023, @paulsemel wrote:

I got a build fail here : http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/10648
As for now, I still don't know why I'm getting this error, but I guessed it was because I was passing the raw_stream..
This should fix this, I hope

In D48807#1149775, @zturner wrote:

I've wanted this for quite a long time. My original motivation for this was back when i was working primarily in LLDB. There's a lot of places where void* is passed around as a generic parameter to a user callback. So, for example, you say DoSomethingAsynchronously(Callback, Param) and when the thing is finished, it invokes Callback(Param). There are obviously better ways to do this if you're designing this from the beginning, but it's not always easy to retrofit that into an existing design. So I think several places in LLDB could be cleaned up by replacing some void* with Any. Note those places still exist today, so Any could be useful there right now.

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?

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).

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.

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)

Looks pretty good to me - nice work!

In D48348#1143128, @Meinersbur wrote:

Getting the size may require iterating the sequence in advance, which I tried to avoid in D48100.

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))

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!

In D48426#1139823, @rnk wrote:

LangOpts.ModulesCodegen is very related in spirit to this, but I think we need a distinct option because that was designed to handle all inline functions (too much), not just dllexport inline functions. + @dblaikie

*looks at this a bit more closely*

Sounds good - thanks!

Yeah, I know nothing about this dump feature or what's being fixed here - test cases would be great to help motivate/explain.

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)

Could you add a test case?

Could you add a test case?

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?

Seems good to me (:

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)

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.

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.

Seems good to me

Hey Zach,

Seems OK to me (:

Thanks for adding some test coverage! Few things that might be able to be simplified before committing.

In D47836#1124278, @craig.topper wrote:

SmallSet<T*> has a template specialization that just inherits from SmallPtrSet.

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?