- User Since
- Oct 8 2012, 9:19 AM (454 w, 3 d)
Ah - this is an alternative to D104082? OK. Yeah, looks like it'd address the thing we were discussing in D98799. But do you have some more details on why D104082 wasn't the right direction? I thought we found other places that used a default-constructed/null GlobalDecl() for StartFunction` correctly, so I'm wondering why this would be a problem in this particular case(s)?
Do you have any interest in seeing if gdb could/would be fixed to handle the imported declaration style? Might be worth knowing if that's feasible, if they have some ideas about if/why that would be a bad idea, etc.
Sounds good, thanks!
@aaron.ballman Do we have attributes/infrastructure for attributes that have to be the same from their first declaration or at least from their first call? I'm wondering if it might be simpler/better to require nodebug to be provided earlier, rather than fixing it up after the fact like this.
Probably worth some test coverage, if possible - does this allow these transformations to be tested with opaque pointers now, or would we have to resort to unit tests to verify this new functionality?
Tue, Jun 22
Huh, that surprises me - guess gdb favors checking the symbol first. I guess maybe it is using something that determines that that symbol comes from the file with debug info - because on a similar test case (one file without debug info, defining some global variable i, another file with debug info with a using ns::i in the global scope - printing i when stepping into the second file correctly prints the using based alias value, but stepping into the file without debug info and printing i complains about not knowing the type of that i)
Mon, Jun 21
This'll need a test case & does the change pass all existing tests?
Also, the patch description could use more detail - it can refer to the bug for more context, but there should be enough detail in the patch title/description to understand the general purpose, etc. (and you can shorten the bug reference to "PR50774" rather than the whole bugs.llvm.org URL)
One of the concerns I'd have, for instance (have you done some broad testing of these patches on sizable code bases?) is that it wouldn't surprise me if clang had some scalability bugs/issues with very long source lines - so it might be necessary to introduce some (arbitrary?) newlines to break up the code. Though I'm not sure - no need to do that pre-emptively, but might be good to have some data that indicates whether this might be a problem or not.
If you create a review using arc's --draft (I think that's the name/spelling) it'll avoid sending email to the *-dev list, so you can create it and share it around without adding more email to the busy *-dev lists until you're ready to send it for review, btw.
What situation are you dealing with where OptBisect is created during global constructors? OptBisect uses ManagedStatic which only creates the object on first access, right? So maybe it'd be suitable to change the code so it doesn't try to access OptBisect during global construction?
This is probably more @aaron.ballman 's wheelhouse, but for my money this seems pretty problematic - will make quoted text in compiler diagnostics weird/difficult to read, etc.
"arguably incorrect IR" isn't a phrase that is especially comforting. It is or isn't, and should have well defined behavior (including being explicitly undefined, if that's the case). Any chance we can get a clearer statement one way or the other?
Fri, Jun 18
@rsmith @dexonsmith @aaron.ballman - folks who might have some opinions on C++ API design. Any thoughts on this patch in general, but also in particular how the discriminator is represented in supported classes? (I'd like to avoid macros, and there are a few tradeoffs around that)
Sounds OK to me.
Sounds good, if this already passes all tests/etc (ie: there actually aren't any callers passing empty strings/non-failed errors/etc)
Thu, Jun 17
Looks good, thanks!
Might make sense to have someone from the committee that approved the proposal sign off on these files being checked into this location (with no review of the content/quality of the files) - and perhaps to commit at that point (regardless of the content/quality of the files) - then do the actual semantic review separately.
This might be a case of an overly reduced patch - this patch alone I'd expect to be observable in some way, and tested. If the "instring.empty()" condition never fires, then I'd expect to change that to an assert, rather than changing the semantics of it in an unobservable way.
Wed, Jun 16
Tue, Jun 15
You can mark it abandoned by choosing that action just above the feedback text box
what're the rules about module level attributes like this? For instance: Does this affect/hinder inlining (if the attributes don't match between the caller and callee)? Other situations that might matter?
Can you instead use the API more like the way llvm-symbolizer does - starting with the skeleton CU in the executable and it'll be able to load the dwp itself? (this codepath is already present and more tested, covers other sections like rnglists, etc, that you'll need too)
The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?
Mon, Jun 14
(generally looks like the right direction to me - sounds like @aeubanks has some good feedback/issues to address)
Looks good, thanks!
Sounds OK - though might be worth considering #including "GraphTraits.h" rather than the forward declaration - makes maintenance a bit easier later on, if default template arguments were added to GraphTraits or other changes are made.
Looks right to me
DICompositeType has a "SizeInBits" field - can that be used to specify a size of 0, instead of adding a new flag to convey this?
Sat, Jun 12
Fri, Jun 11
Sounds good to me - couple of optional comments/questions that could be clarified in the text.
Thu, Jun 10
Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't.
Does the outliner only run for commoning code, or can it run on a single function (for hot/cold reasons, or anything else)?
Tue, Jun 8
Mon, Jun 7
This looks about right to me - probably follow-up on the email threads to mention this has been sent for review/link to this review?
Fri, Jun 4
Looks good, thanks!
Thu, Jun 3
Is the C source syntax already fixed? Could it use a more specific name - then maybe wouldn't need the flag to opt-in if the only places the attribute appears is where it should be propagated?