[PATCH] D92606: Clean up debug locations for logical-and/or expressions

Thu Dec 3 14:44:44 PST 2020

dblaikie added a comment.

Probably worth separating these changes/fixes/tests (looks like 3 different changes?) - at least it'd help me understand what each one of the changes does, separately (I'm especially curious about the EmitScalarConversion one and would like to better understand why the generic Expr handling for ApplyDebugLocation isn't working here, if it can be made more robust and/or if the proposed solution could be generalized a bit/moved further up in the stack). And is there an existing test file/cases these could be put near?

================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4293
+  {
+    // There is no need to emit line number for unconditional branch.
+    auto NL = ApplyDebugLocation::CreateEmpty(CGF);
----------------
rnk wrote:
> I see this is consistent with LAnd handling above, but is there a principled reason for using an empty location here? If I was stopped on this unconditional branch instruction in the debugger, I would expect to see the location of the logical operator.
> 
> No need to change behavior in this patch, I'm just wondering if we can remove this as a followup.
> 
> I dug into the blame, and the comment about "no need" for a location comes from 2011:
> http://github.com/llvm/llvm-project/commit/4d7612744f080d2c0a8639c1f5e41b691e1bba55
> The reasoning seems to be that it improves the gdb test suite.
I /think/ I remember encountering these issues independently/as well when I was working on the gdb test suite maybe a couple of years later. Hmm, guess it might've overlapped with this work.

6f2e41e0d4421ab376801bfb88d3f197a8e96994 / r128471 provides maybe a bit more context: "Do not line number entry for unconditional branches. Usually, users do not want to stop at closing '}'."

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92606/new/

https://reviews.llvm.org/D92606