Add option to generate additional debug info for expression dereferencing pointer to pointers.

[huangjd]

This is another attempt to land #81545, which was reverted.

Fixed test case so that it is consistent on all platforms.

[dwblaikie]

Looks worth a shot. Please commit this at a time where you can respond to bot fail-mail for a while after the commit.

And ensure the commit message has most/all of the details of the original commit, plus the original commit and revert hashes, and a little detail on the updates to address the reason for the revert (adding the explicit triple to the test).

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 9261ab708e37c2d6499ac063045f816d25a5919c 4841c8752abab8c5e05a97d8826cdca376f4ab0c -- clang/test/CodeGenCXX/debug-info-ptr-to-ptr.cpp clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/CGDebugInfo.h clang/lib/CodeGen/CGExprScalar.cpp

View the diff from clang-format here.

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 6c332b2ae9c..2ed7dbe2854 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -5706,9 +5706,9 @@ void CGDebugInfo::EmitPseudoVariable(CGBuilderTy &Builder,
   // Emit debug info for materialized Value.
   unsigned Line = Builder.getCurrentDebugLocation().getLine();
   unsigned Column = Builder.getCurrentDebugLocation().getCol();
-  llvm::DILocalVariable *D = DBuilder.createAutoVariable(
-      LexicalBlockStack.back(), "", nullptr, 0, Type, false,
-      llvm::DINode::FlagArtificial);
+  llvm::DILocalVariable *D =
+      DBuilder.createAutoVariable(LexicalBlockStack.back(), "", nullptr, 0,
+                                  Type, false, llvm::DINode::FlagArtificial);
   llvm::DILocation *DIL =
       llvm::DILocation::get(CGM.getLLVMContext(), Line, Column,
                             LexicalBlockStack.back(), CurInlinedAt);

[jmorse]

G'day -- we've got some tests for -O0 that, with this patch applied, generate different code with-and-without the -g flag, if -fdebug-info-for-profiling is enabled. Example godbolt: https://godbolt.org/z/qooo5Eah1 .

It seems the intention of this patch is generating additional loads and stores with debug-info enabled, which is usually highly undesirable. It's guarded by -fdebug-info-for-profiling, however we (Sony) use this flag in a bunch of places, and would prefer it to continue generating stable + consistent code. A couple of possibilities:

• Put this behaviour behind a cc1 flag, I think that was the plan in the original PR?
• Instead of creating an alloca, store+load, and dbg.declare, connect a dbg.value to the relevant pointer Value.

The 2nd point is more work, but it means you're only adding more debug-info, and nothing else. A dbg.value connected to the pointer Value is the inevitable consequence of the alloca+store+load idiom after mem2reg/SROA anyway.

[dwblaikie]

G'day -- we've got some tests for -O0 that, with this patch applied, generate different code with-and-without the -g flag, if -fdebug-info-for-profiling is enabled. Example godbolt: https://godbolt.org/z/qooo5Eah1 .

It seems the intention of this patch is generating additional loads and stores with debug-info enabled, which is usually highly undesirable.

I hope that isn't the intent, I thought the intent was just to add more debug info for loads and stores to carry more intermediate type information.

Sorry I missed that this patch does change clangs generated ir. Seriously overlooked on my part.

• Instead of creating an alloca, store+load, and dbg.declare, connect a dbg.value to the relevant pointer Value.

The 2nd point is more work, but it means you're only adding more debug-info, and nothing else. A dbg.value connected to the pointer Value is the inevitable consequence of the alloca+store+load idiom after mem2reg/SROA anyway.

Yeah, pretty sure (2) would be the preferred direction.

[huangjd]

By just specifying -g Even without -fdebug-info-for-profiling it is going to introduce debug variables as a sequence of alloca-store-load too, so is it a requirement to guarantee -O0 output stays identical with or without debug info?

Emitting a dbg.value was what I originally though of, but there is a IR validation pass after frontend codegen that does not permit it, not sure if there's other design reason for this check

[jmorse]

dblaikie wrote:

I hope that isn't the intent, I thought the intent was just [...]

Ah, by which I mean "it's not an accident",

By just specifying -g Even without -fdebug-info-for-profiling it is going to introduce debug variables as a sequence of alloca-store-load too, so is it a requirement to guarantee -O0 output stays identical with or without debug info?

It's a strong objective of LLVM in general to ensure debug-info doesn't have an effect on codegen decisions -- it just makes everything unreliable and hard to predict. It can cause situations where an end user finds their programs behave differently with -g. Having a cc1 option to enable this kind of behaviour is fine as they're relatively experimental, for us the issue is that it's affecting an existing driver option.

Emitting a dbg.value was what I originally though of, but there is a IR validation pass after frontend codegen that does not permit it, not sure if there's other design reason for this check

Interesting -- I'll admit I'm not especially familiar with stuff right at the front end, but I'm sure I've seen some emails go past about handling dbg.values earlier than normal. @SLTozer @OCHyams were there some Swift related patches that got SROA handling dbg.values correctly, and if so, maybe it's alright for clang to do it too? (@jmorse is still catching up with email).

[SLTozer]

Yes, to my knowledge Swift emits dbg.values out of its front end, so there's probably no reason (on the LLVM side) that clang couldn't do so as well. There might be good reasons internal to clang that it doesn't use debug values atm, and it's not inconceivable that we'd hit some cases that LLVM handles poorly at the moment (right now assignment tracking depends on reading dbg.declares, for example), but that's something that could be tackled in a separate review.

In any case, as has been said this is definitely an error and should be reverted - the best solution is probably to rework this to use dbg.values instead.

[huangjd]

Ok lets revert this first, and I will work on the dbg.value implementation