Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL] ccache is invalidated by random file names generated for offloaded code bundles #5260

Open
psalz opened this issue Jan 5, 2022 · 14 comments
Assignees
Labels
bug Something isn't working

Comments

@psalz
Copy link
Contributor

psalz commented Jan 5, 2022

Describe the bug

In its default "direct mode", ccache examines the preprocessor output to determine all dependencies for a given translation unit. In doing so, ccache gets tripped up by the randomly generated filenames for offloaded code bundles. The preprocessor output for a small example program will look something like this (excerpt):

// __CLANG_OFFLOAD_BUNDLE____START__ host-x86_64-unknown-linux-gnu
# 1 "/tmp/test-963ab6.cpp"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 404 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "/tmp/test-header-08e0d3.h" 1
# 2 "<built-in>" 2
# 1 "/tmp/test-963ab6.cpp" 2
# 1 "test.cpp"
int main() { return 0; }
# 1 "/tmp/test-footer-cd3679.h" 1
# 4 "test.cpp" 2

Here, the files /tmp/test-963ab6.cpp, /tmp/test-header-08e0d3.h, /tmp/test-963ab6.cpp and /tmp/test-footer-cd3679.h have randomly generated names that change with each compiler invocation, causing the cache to be invalidated every time.

I assume this is a regression as according to #1797 ccache should work with DPC++.

NOTE: This can be worked around by using ccache's "depend mode" (CCACHE_DEPEND=1), which does not parse preprocessor output to determine dependencies, relying on compiler-generated dependency files instead (-MD/-MMD).

To Reproduce

Create file test.cpp:

int main() { return 0; } 

Run DPC++ with ccache (at least twice):

CCACHE_LOGFILE="$PWD/ccache.log" ccache clang++ -fsycl test.cpp -c -o test.o

Examining the produced ccache.log file will reveal something like this:

...
[2022-01-05T11:51:30.485895 3877683] No b8b4653v0ceorrbv3ra96m7q5m0hd13he in primary storage
[2022-01-05T11:51:30.485942 3877683] Running preprocessor
[2022-01-05T11:51:30.485948 3877683] Executing clang++ -fsycl -E test.cpp
[2022-01-05T11:51:30.551676 3877683] Failed to stat /tmp/test-3f740e.cpp: No such file or directory
[2022-01-05T11:51:30.551697 3877683] Disabling direct mode
...

As you can see, not only are the random names problematic, ccache is additionally unable to access the files which presumably get deleted right away. I'm therefore wondering whether there is any advantage (for tooling) in emitting these file names into the preprocessor output in the first place, or whether they could simply be removed.

Environment:

  • OS: Ubuntu 20.04
  • DPC++ version: clang version 14.0.0 (https://github.com/intel/llvm ec97c573cdf684aec779d19ac68f1e9470ca0516)
  • ccache version: 4.5.1 (also tested with 3.7.7)
@psalz psalz added the bug Something isn't working label Jan 5, 2022
@AlexeySachkov
Copy link
Contributor

This is caused by our integration footer implementation: we append a piece of code at the end of user-provided input and we are doing this though creating a temporary file. If my assumption is correct, then adding -fno-sycl-use-footer should help to workaround this issue.

I'm therefore wondering whether there is any advantage (for tooling) in emitting these file names into the preprocessor output in the first place, or whether they could simply be removed.

Integration footer is used by implementation of some both core SYCL 2020 features and Intel extensions. At the moment, the list includes specialization constants, is_device_copyable and SYCL_INTEL_device_global.

Tagging @mdtoguchi here as well. I guess it is possible to generate more stable name for those temporary files, but I'm afraid there could be issues if the same files are being compiled several times at the same time - temporary files would be the same and if compiler options are different in those compilations, that will lead to unexpected results.

@psalz
Copy link
Contributor Author

psalz commented Jan 11, 2022

I'm therefore wondering whether there is any advantage (for tooling) in emitting these file names into the preprocessor output in the first place, or whether they could simply be removed.

Integration footer is used by implementation of some both core SYCL 2020 features and Intel extensions. At the moment, the list includes specialization constants, is_device_copyable and SYCL_INTEL_device_global.

What I meant is whether there is any real use case for emitting the linemarkers for those temporary files, because that's what's tripping up ccache. AFAIK these are usually needed to properly report errors (with correct file name / line numbers), but I wonder whether there is any value in getting errors for temporary files that no longer exist once the error message is shown to the user. But of course there may be other uses for those linemarkers that I don't know of.

@AlexeySachkov
Copy link
Contributor

What I meant is whether there is any real use case for emitting the linemarkers for those temporary files, because that's what's tripping up ccache. AFAIK these are usually needed to properly report errors (with correct file name / line numbers), but I wonder whether there is any value in getting errors for temporary files that no longer exist once the error message is shown to the user. But of course there may be other uses for those linemarkers that I don't know of.

I don't know exactly why are they needed here, but my guess is that besides diagnostic messages they help to generate the correct debug info. Tagging @premanandrao and @mdtoguchi to help here. I'm also not sure how useful it is to have the correct debug info for int-footer/int-header, because hopefully end user won't need to debug those parts of the toolchain.

@mdtoguchi, do you think that this issue can be avoided if we change the implementation of integration footer?
From:

clang++ -cc1 /* emit-int-footer=footer.h */
append-file main.cpp footer.h -o tmp.cpp
clang++ -cc1 tmp.cpp

To:

clang++ -cc1 /* emit-int-footer=footer.h */
clang++ -cc1 -include main.cpp footer.h -main-file-name main.cpp

@mdtoguchi
Copy link
Contributor

Modifying the compilation to use the footer as the main file and including the source file as an -include is do-able in the compiler driver. It is unclear to me what kind of impact this will have down the line.

@premanandrao
Copy link
Contributor

What I meant is whether there is any real use case for emitting the linemarkers for those temporary files, because that's what's tripping up ccache. AFAIK these are usually needed to properly report errors (with correct file name / line numbers), but I wonder whether there is any value in getting errors for temporary files that no longer exist once the error message is shown to the user. But of course there may be other uses for those linemarkers that I don't know of.

I don't know exactly why are they needed here, but my guess is that besides diagnostic messages they help to generate the correct debug info. Tagging @premanandrao and @mdtoguchi to help here. I'm also not sure how useful it is to have the correct debug info for int-footer/int-header, because hopefully end user won't need to debug those parts of the toolchain.

@mdtoguchi, do you think that this issue can be avoided if we change the implementation of integration footer? From:

clang++ -cc1 /* emit-int-footer=footer.h */
append-file main.cpp footer.h -o tmp.cpp
clang++ -cc1 tmp.cpp

To:

clang++ -cc1 /* emit-int-footer=footer.h */
clang++ -cc1 -include main.cpp footer.h -main-file-name main.cpp

Tagging @bwyma for his opinion on the debug question.

@ax3l
Copy link

ax3l commented Mar 31, 2023

Since the issue is inactive since a year, this is just an unqualified comment that we would love to see ccache support back in action :)

Thanks for the deep dive already!

@KornevNikita
Copy link
Contributor

KornevNikita commented May 17, 2024

Hi! There have been no updates for at least the last 60 days, though the ticket has assignee(s).

@AlexeySachkov, could I ask you to take one of the following actions? :)

  • Please provide an update if you have any (or just a small comment if you don't have any yet).
  • OR mark this issue with the 'confirmed' label if you have confirmed the problem/request and our team should work on it.
  • OR close the issue if it has been resolved.
  • OR take any other suitable action.

Thanks!

Copy link
Contributor

Hi! There have been no updates for at least the last 60 days, though the issue has assignee(s).

@AlexeySachkov, could you please take one of the following actions:

  • provide an update if you have any
  • unassign yourself if you're not looking / going to look into this issue
  • mark this issue with the 'confirmed' label if you have confirmed the problem/request and our team should work on it
  • close the issue if it has been resolved
  • take any other suitable action.

Thanks!

@mdtoguchi
Copy link
Contributor

There is work in progress to improve this behavior. It is to use a new '-include-footer' option to alleviate the temporary file generation for the footer. #14402

Copy link
Contributor

Hi! There have been no updates for at least the last 60 days, though the issue has assignee(s).

@AlexeySachkov, could you please take one of the following actions:

  • provide an update if you have any
  • unassign yourself if you're not looking / going to look into this issue
  • mark this issue with the 'confirmed' label if you have confirmed the problem/request and our team should work on it
  • close the issue if it has been resolved
  • take any other suitable action.

Thanks!

@psalz
Copy link
Contributor Author

psalz commented Sep 16, 2024

There is work in progress to improve this behavior. It is to use a new '-include-footer' option to alleviate the temporary file generation for the footer. #14402

I just tested my reproducer example from above with a recent (ed2128d) build of DPC++ and I'm getting the same behavior. Do I have to pass any additional compiler flags?

@mdtoguchi
Copy link
Contributor

@psalz, we have discovered additional changes that are needed to address the addition of the internal header/footer files (referenced in the generated preprocessed file). This is expected to be fixed for the DPC++ 2025.1 release.

Copy link
Contributor

Hi! There have been no updates for at least the last 60 days, though the issue has assignee(s).

@AlexeySachkov, could you please take one of the following actions:

  • provide an update if you have any
  • unassign yourself if you're not looking / going to look into this issue
  • mark this issue with the 'confirmed' label if you have confirmed the problem/request and our team should work on it
  • close the issue if it has been resolved
  • take any other suitable action.

Thanks!

@mdtoguchi
Copy link
Contributor

@psalz, The additional work as mentioned has been performed: #15634, could you verify?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants