Diff Detail
Event Timeline
Please add the debug-info tag to these reviews.
I know off-list I recommended doing all your transform-related patches separately, but... given that this and D73473 are both in SROA, I think the tests at least should be combined into one test file (which reduces testing overhead). And in general it should be fine to combine patches for the same pass into a single patch.
I'd probably still vote for separate patches for separate fixes - better for bisection, etc. (separate test cases in the same test file's OK though - different IR functions that exercise each different path) But wouldn't quibble over this specific instance (not worth the churn of back and forth between split/merged/etc) - just for future reference/my 2c.
Also - it might be handy to describe when/where/how this particular change is important. While testing in isolation like this is probably the right thing - at least in this example, while it has an effect on the generated IR, it doesn't seem to have an effect on the resulting object file. (either way we still get no locations for these variables because they get entirely optimized away) - I'm guessing the case where it's observable in the resulting DWARF and debugger experience is if there's another codepath that does keep a location - without your changes, that other location would maybe wash out/extend over this range, where it should really be undef instead. (so including such an example in the comments/commit message - not as a test case, might be illustrative for review/historic purposes)
Hi @dblaikie, thanks for your comments.
At least in this example, while it has an effect on the generated IR, it doesn't seem to have an effect on the resulting object file
Now you mention it I think a couple of these I have lined up should not ever affect the object file, but do, because of this:
Good: clang -O2 -g -emit-llvm ex.c -o - adds all DILocalVariables to the DISubprogram retainedNodes list (even when the DILocalVariable still has other uses).
Bad: clang ex.c -O0 -g -emit-llvm -Xclang -disable-O0-optnone -o - | opt -O2 -S does not on trunk, but does with my changes because the undef-instead-of-dropped dbg.value uses the DILocalVariable metadata.
At a glance it looks like running at >O0 will add all DILocalVariables to a DISubprogram retainedNode list, and running at O0 does not.
Not sure if this is a bug or something I just wasn't aware of. I was compiling all of these test cases with the 'Bad' build line above, so any dropped dbg.values which were the last users of the DILocalVariable metadata caused the local variable to be omitted from the final dwarf.
Oh, I noticed you'd generated your tests that way and was going to suggest using -O2 -Xclang -disable-llvm-passes instead, but didn't think it'd matter - but you're quite right, that Clang/LLVM does behave differently, by adding the "variables:" list to the DISubprogram when optimizing. This is intentional, to ensure local variables (& especially parameters) are not lost due to optimizations.
So how did you come to be trying to fix these bugs, if they seem to be not observable through the normal usage of Clang/LLVM? Perhaps we shouldn't fix them if there's no way for them to be problematic? (though I suspect with some work you might be able to find user-visible bugs at these sources like the sort of path I described (where the transformation is currently removing dbg value intrinsics - in these cases where none remain it's probably benign, but if there were other dbg values in other codepaths that might otherwise be clobbered by the undef, without it they might spill over & start producing misleading locations in unrelated pieces of code)
So how did you come to be trying to fix these bugs?
I was specifically looking for cases where debug intrinsics are dropped.
Perhaps we shouldn't fix them
That's fair for the ones like this which will never affect the final debuginfo.
As you have pointed out my build commands have turned up some false positives. I'll sort through the fixes (and update tests so that they use and work with -disable-llvm-passes) and upload them when I get the chance.
Hi @Orlando, sorry, I am a bit confused, do you plan to create new set of patches addressing this (and related cases), or you will update this one? :)