intel
[SYCL] Fix debug info generation when integration footer is present
Two changes were made:
Driver passes an absolute path to an original source file through -main-file-name if integration footer is enabled. Clang CodeGen uses the file specified through -main-file-name to calculate checksum for the main file, if -fsycl-use-main-file-name option is passed. These changes help to fix two known issues with debugging caused by integration footer presence, without redesigning the integration footer approach.
Those issues are:
several conflicting entries in .debug_line section, causing gdb l 1,100 command to fail:
(gdb) l 1,100
Specified first line '1' is ambiguous:
file: "/tmp/t.cpp", line number: 1, symbol: "???"
file: "t.cpp", line number: 1, symbol: "???"
missing checksum for the main file on Windows, causing breakpoints not being hit.
Joint work with @AlexeySachkov
A couple of minor nits. But one question: does the footer file match the format of the actual footer file generated (for #line directives and #include directives)?
Yes it does. I actually copied that from an output of a run of dpcpp -fsycl -O0 checksum.cpp.
Can any of you please take at look at this PR? It has been sitting here for a while. @AlexeySachkov @stevemerr @bwyma
I tested this change on Windows and it does produce the desired results in the debug info and the desired debugger behavior with Visual Studio, so from that perspective I'm satisfied with the change.
Can any of you please take at look at this PR? It has been sitting here for a while.
If you need an LLVM Debug Information review then please add me as a reviewer. This patch still has unresolved issues from 22 days ago and isn't green yet. Is it ready for review?
Can any of you please take at look at this PR? It has been sitting here for a while.
If you need an LLVM Debug Information review then please add me as a reviewer. This patch still has unresolved issues from 22 days ago and isn't green yet. Is it ready for review?
Yes it's ready for review. Thanks.
@bader There is something strange here. When I click on details for the fails above and do a re-starts it terminates with SUCCESS but the PR doesn't get updated. http://llvm-ci-test2.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FPrecommit_Check_User/detail/Precommit_Check_User/1307/pipeline
Am I missing something?
@bader There is something strange here. When I click on details for the fails above and do a re-starts it terminates with SUCCESS but the PR doesn't get updated. http://llvm-ci-test2.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FPrecommit_Check_User/detail/Precommit_Check_User/1307/pipeline
Am I missing something?
@tfzhu, could you help with these Jenkins issues?
We need separate source files for the customer source and the integration footer in the same way we have a separate source file for the integration header. Copying the original source over to the temp directory, changing it, and pretending it is the original source won't work because the temp file does not match the original source. Anything added to the temp file for the integration footer which generates code with source correlation invalid for the original file will confuse the debugger and will be a blocking issue for performance analysis tools. I pointed this out some months ago already.
Although we don't currently support code coverage, we do support performance analysis tools. I am happy this patch will resolve one of the more annoying source correlation issues when debugging, but as soon as any code from the integration footer shows up during performance analysis and can't be attributed back to the original source file you're likely to get defects for this design again. I just want to make sure we're all on the same page about this.
@stevemerr , @bwyma Where are we at with this? This patch still needs approval from one ? Can we move forward one way or another for this? @tfzhu has confirmed that the fail in this PR is not related to this patch. Thanks.
@stevemerr , @bwyma Where are we at with this? This patch still needs approval from one ? Can we move forward one way or another for this? @tfzhu has confirmed that the fail in this PR is not related to this patch. Thanks.
Did you have a comment regarding the issue I raised?
@stevemerr , @bwyma Where are we at with this? This patch still needs approval from one ? Can we move forward one way or another for this? @tfzhu has confirmed that the fail in this PR is not related to this patch. Thanks.
Did you have a comment regarding the issue I raised? I understand that the issue is not fully solved. This patch is solving only one aspect of a serious issue with footer. Should this patch be rejected? Or completed with additional design? I just want to move forward with this. Thanks.
You didn't rebase this patch file and it no longer applies cleanly to xmain head.
I think we need to review the testing plan for these changes as they don't appear to resolve the original problem:
$ dpcpp -c -O0 -g array-transform.cpp
$ dpcpp -o O0-array-transform.exe -O0 -g array-transform.o -lOpenCL
$ gdb-oneapi O0-array-transform.exe
GNU gdb (Intel(R) Distribution for GDB* 2022.2) 10.2
...
Reading symbols from O0-array-transform.exe...
(gdb) l 1,100
Specified first line '1' is ambiguous:
file: "/tmp/array-transform.cpp", line number: 1, symbol: "???"
file: "array-transform.cpp", line number: 1, symbol: "???"
(gdb)
If I applied the patch incorrectly, then I would have expected the LIT testing to fail:
$ llvm-lit llvm/clang/test/CodeGenSYCL/debug-info-checksum-temp-name.cpp \
llvm/clang/test/CodeGenSYCL/debug-info-file-checksum.cpp \
llvm/clang/test/Driver/sycl-int-footer.cpp
-- Testing: 3 tests, 3 workers --
PASS: Clang :: CodeGenSYCL/debug-info-file-checksum.cpp (1 of 3)
PASS: Clang :: CodeGenSYCL/debug-info-checksum-temp-name.cpp (2 of 3)
PASS: Clang :: Driver/sycl-int-footer.cpp (3 of 3)
Testing Time: 0.72s
Passed: 3
If you apply this patch file to dump out the file/dir being created by CLANG then it may help with testing:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 370a7f282aa1..359df38e990f 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -666,6 +666,7 @@ void CGDebugInfo::CreateCompileUnit() {
llvm::DIFile *CUFile = DBuilder.createFile(
remapDIPath(MainFileName), remapDIPath(getCurrentDirname()), CSInfo,
getSource(SM, SM.getMainFileID()));
+ CUFile->dump(); // BROCK
StringRef Sysroot, SDK;
if (CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::LLDB) {
Compiling an example using the patch above you will see what CLANG is sending to the debug information for the host and target when using a relative path:
$ dpcpp -c -O0 -g array-transform.cpp
<0x10c26b00> = !DIFile(filename: "array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton")
<0x10e38580> = !DIFile(filename: "/tmp/array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton")
... and when using an absolute path:
$ dpcpp -c -O0 -g $PWD/array-transform.cpp
<0x11d88c60> = !DIFile(filename: "/localdisk2/bwyma/zahira/test/singleton/array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton")
<0x11d82e20> = !DIFile(filename: "/tmp/array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton")
You didn't rebase this patch file and it no longer applies cleanly to xmain head. I think we need to review the testing plan for these changes as they don't appear to resolve the original problem:
$ dpcpp -c -O0 -g array-transform.cpp $ dpcpp -o O0-array-transform.exe -O0 -g array-transform.o -lOpenCL $ gdb-oneapi O0-array-transform.exe GNU gdb (Intel(R) Distribution for GDB* 2022.2) 10.2 ... Reading symbols from O0-array-transform.exe... (gdb) l 1,100 Specified first line '1' is ambiguous: file: "/tmp/array-transform.cpp", line number: 1, symbol: "???" file: "array-transform.cpp", line number: 1, symbol: "???" (gdb)If I applied the patch incorrectly, then I would have expected the LIT testing to fail:
$ llvm-lit llvm/clang/test/CodeGenSYCL/debug-info-checksum-temp-name.cpp \ llvm/clang/test/CodeGenSYCL/debug-info-file-checksum.cpp \ llvm/clang/test/Driver/sycl-int-footer.cpp -- Testing: 3 tests, 3 workers -- PASS: Clang :: CodeGenSYCL/debug-info-file-checksum.cpp (1 of 3) PASS: Clang :: CodeGenSYCL/debug-info-checksum-temp-name.cpp (2 of 3) PASS: Clang :: Driver/sycl-int-footer.cpp (3 of 3) Testing Time: 0.72s Passed: 3If you apply this patch file to dump out the file/dir being created by CLANG then it may help with testing:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp index 370a7f282aa1..359df38e990f 100644 --- a/clang/lib/CodeGen/CGDebugInfo.cpp +++ b/clang/lib/CodeGen/CGDebugInfo.cpp @@ -666,6 +666,7 @@ void CGDebugInfo::CreateCompileUnit() { llvm::DIFile *CUFile = DBuilder.createFile( remapDIPath(MainFileName), remapDIPath(getCurrentDirname()), CSInfo, getSource(SM, SM.getMainFileID())); + CUFile->dump(); // BROCK StringRef Sysroot, SDK; if (CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::LLDB) {Compiling an example using the patch above you will see what CLANG is sending to the debug information for the host and target when using a relative path:
$ dpcpp -c -O0 -g array-transform.cpp <0x10c26b00> = !DIFile(filename: "array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton") <0x10e38580> = !DIFile(filename: "/tmp/array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton")... and when using an absolute path:
$ dpcpp -c -O0 -g $PWD/array-transform.cpp <0x11d88c60> = !DIFile(filename: "/localdisk2/bwyma/zahira/test/singleton/array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton") <0x11d82e20> = !DIFile(filename: "/tmp/array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton")So you are using gdb. I thought that was an issue only with VS debugger. I have to say that I didn't try to do anything with gdb. Were you able to test the changes on Windows?
You didn't rebase this patch file and it no longer applies cleanly to xmain head.
I think we need to review the testing plan for these changes as they don't appear to resolve the original problem:
$ dpcpp -c -O0 -g array-transform.cpp $ dpcpp -o O0-array-transform.exe -O0 -g array-transform.o -lOpenCL $ gdb-oneapi O0-array-transform.exe GNU gdb (Intel(R) Distribution for GDB* 2022.2) 10.2 ... Reading symbols from O0-array-transform.exe... (gdb) l 1,100 Specified first line '1' is ambiguous: file: "/tmp/array-transform.cpp", line number: 1, symbol: "???" file: "array-transform.cpp", line number: 1, symbol: "???" (gdb)If I applied the patch incorrectly, then I would have expected the LIT testing to fail:
$ llvm-lit llvm/clang/test/CodeGenSYCL/debug-info-checksum-temp-name.cpp \ llvm/clang/test/CodeGenSYCL/debug-info-file-checksum.cpp \ llvm/clang/test/Driver/sycl-int-footer.cpp -- Testing: 3 tests, 3 workers -- PASS: Clang :: CodeGenSYCL/debug-info-file-checksum.cpp (1 of 3) PASS: Clang :: CodeGenSYCL/debug-info-checksum-temp-name.cpp (2 of 3) PASS: Clang :: Driver/sycl-int-footer.cpp (3 of 3) Testing Time: 0.72s Passed: 3If you apply this patch file to dump out the file/dir being created by CLANG then it may help with testing:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp index 370a7f282aa1..359df38e990f 100644 --- a/clang/lib/CodeGen/CGDebugInfo.cpp +++ b/clang/lib/CodeGen/CGDebugInfo.cpp @@ -666,6 +666,7 @@ void CGDebugInfo::CreateCompileUnit() { llvm::DIFile *CUFile = DBuilder.createFile( remapDIPath(MainFileName), remapDIPath(getCurrentDirname()), CSInfo, getSource(SM, SM.getMainFileID())); + CUFile->dump(); // BROCK StringRef Sysroot, SDK; if (CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::LLDB) {Compiling an example using the patch above you will see what CLANG is sending to the debug information for the host and target when using a relative path:
$ dpcpp -c -O0 -g array-transform.cpp <0x10c26b00> = !DIFile(filename: "array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton") <0x10e38580> = !DIFile(filename: "/tmp/array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton")... and when using an absolute path:
$ dpcpp -c -O0 -g $PWD/array-transform.cpp <0x11d88c60> = !DIFile(filename: "/localdisk2/bwyma/zahira/test/singleton/array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton") <0x11d82e20> = !DIFile(filename: "/tmp/array-transform.cpp", directory: "/localdisk2/bwyma/zahira/test/singleton")
@bwyma Back on this. I don't know how to build the executable gdb-oneapi. I added the call to dump in CreateCompileUnit() like you suggested above and I get these DICompileUnit. I am assuming that for the first line is on host, and the second line is on target?
ksh-3.2$ ./clang++ -c -fsycl -O0 -g d:/IUSERS/zahiraam/Testing/hello.cpp <0x229d3872398> = !DIFile(filename: "d:/IUSERS/zahiraam/Testing\hello.cpp", directory: "d:\IUSERS\zahiraam\my-sycl-bp\build\Debug\bin", checksumkind: CSK_MD5, checksum: "5b7fdfcc4aaf8d4a6133c9d32a6d3fb8") <0x1a8ab67f978> = !DIFile(filename: "C:\Users\zahiraam\AppData\Local\Temp\4\hello.cpp", directory: "d:\IUSERS\zahiraam\my-sycl-bp\build\Debug\bin", checksumkind: CSK_MD5, checksum: "7e7310a583339a7d5ed72573a13d7959")
ksh-3.2$ ./clang++ -c -fsycl -O0 -g hello.cpp <0x1730438fed8> = !DIFile(filename: "hello.cpp", directory: "d:\IUSERS\zahiraam\my-sycl-bp\build\Debug\bin", checksumkind: CSK_MD5, checksum: "5b7fdfcc4aaf8d4a6133c9d32a6d3fb8") <0x27984a820c8> = !DIFile(filename: "C:\Users\zahiraam\AppData\Local\Temp\4\hello.cpp", directory: "d:\IUSERS\zahiraam\my-sycl-bp\build\Debug\bin", checksumkind: CSK_MD5, checksum: "4dc976a3cd39f43222adf74269e33cf3") ksh-3.2$
I am not sure I understand what's expected here? Should the filename for the target be the file with the footer? Can you please explain?
I don't know how to build the executable gdb-oneapi.
The Intel OneAPI GDB is available on the R: drive under /rdrive/ref/gdb-oneapi/. But you can use the system GDB too.
I am not sure I understand what's expected here? Should the filename for the target be the file with the footer? Can you please explain?
When you run the driver like this:
ksh-3.2$ ./clang++ -c -fsycl -O0 -g hello.cpp
There is only one user source file you specified.. it is "hello.cpp" in the current working directory. Looking at the DIFiles:
<0x1730438fed8> = !DIFile(filename: "hello.cpp", directory: "d:\IUSERS\zahiraam\my-sycl-bp\build\Debug\bin", checksumkind: CSK_MD5, checksum: "5b7fdfcc4aaf8d4a6133c9d32a6d3fb8") <0x27984a820c8> = !DIFile(filename: "C:\Users\zahiraam\AppData\Local\Temp\4\hello.cpp", directory: "d:\IUSERS\zahiraam\my-sycl-bp\build\Debug\bin", checksumkind: CSK_MD5, checksum: "4dc976a3cd39f43222adf74269e33cf3")
These DIFiles say there are two different files named hello.cpp, one in the current working directory and one in a local temp directory, and the MD5 checksum is different indicating the two files do not contain the same information.
The expectations are:
- The host and target DIFile must refer to the same user-specified source file in the same directory.
- The host and target DIFile will have the same checksum value since they refer to the same user-specified source file.
The integration header/footer are artifacts of the toolchain and should not be represented in the debug information describing the original user source files.