[PATCH] Fixing a bug where debug info for a local variable gets emitted at file scope

[David Blaikie]

On Thu, Aug 29, 2013 at 2:58 PM, Yunzhong Gao
<Yunzhong_Gao at playstation.sony.com> wrote:
>   Hi David,
>   Thanks for reviewing my patch. I try to address your concerns below.
>
>   > lib/CodeGen/CGDebugInfo.cpp#3058
>   > This comment seems suspiciously like we're working around something in a
>   > not-great way, but I don't understand: why are we setting the location at
>   > all here, do you know or did you copy this from elsewhere?
>
>   I do not know either. In the original codes, EmitGlobalVariable() for
>   non-deferred global variables sets source locations; the version for deferred
>   global variables does not set source location. So when I combine these two
>   versions in this patch, I added a condition check to preserve the original
>   code semantics. I tried to disable this particular line of codes, and the
>   test results in LLVM/Clang regression suites were still clean...
>
>   But on the other hand, I think this is not really relevant to this bug, and
>   if we decide to remove this line, it should be done in a separate patch. Is
>   it okay to add a FIXME comment there for now?

Yep - For extra credit, you could check the blame history, see if you
could find out why it was committed, presumably without tests (or
maybe it was committed with tests & this line of code was needed to
pass the test at the time, but something else has addressed this issue
more generally since then) but might have a bug number that gives
hints as to why this is there.

>   > test/CodeGen/debug-info-line5.cpp#2
>   > Clang tests should only test the generated IR without invoking the LLVM
>   > backend to emit object files, etc. If your change is purely in Clang, the
>   > should be possible to test it by only exercising Clang.
>
>   Thank you for pointing it out. Fixed.

Thanks

>
>   > test/CodeGen/debug-info-scope.c#28
>   > Any reason you needed to add this 'foo' function as well as referencing
>   > 'd' at the end of the previous function?
>
>   Not really. I removed this local function.

Thanks

>
>   > test/CodeGenCXX/debug-info-namespace.cpp#39
>   > Thanks for putting all these test cases in reasonable existing files.
>   > Though please try to be consistent with existing formatting in files (you
>   > could continue to use single letter class names "C", "D", etc rather than
>   > monkey/ape - simple, metasyntactic variables can be easy to follow rather
>   > than adding extra unnecessary semantics to the test cases) and usually with
>   > the LLVM style guide (so, '{' at the end of the line, rather than the
>   > beginning)
>
>   Fixed (hopefully).

Looks fine.

Please commit, or I can do so for you if you don't have commit rights.

>
> http://llvm-reviews.chandlerc.com/D1281
>
> CHANGE SINCE LAST DIFF
>   http://llvm-reviews.chandlerc.com/D1281?vs=3583&id=3907#toc
>
> Files:
>   lib/CodeGen/CodeGenFunction.cpp
>   lib/CodeGen/CGDebugInfo.cpp
>   lib/CodeGen/CodeGenFunction.h
>   lib/CodeGen/CGDebugInfo.h
>   lib/CodeGen/CGExpr.cpp
>   test/CodeGen/debug-info-scope.c
>   test/CodeGen/debug-info-line5.h
>   test/CodeGen/debug-info-line5.cpp
>   test/CodeGenCXX/debug-info-namespace.cpp