Page MenuHomePhabricator

OpenMP: Fix object clobbering issue when using save-temps
ClosedPublic

Authored by pdhaliwal on Feb 23 2021, 5:05 AM.

Details

Summary

There are two preconditions to reproduce the issue,

  1. Use -save-temps option
  2. Provide the -o option with name equal to the input file name without the file extension. For e.g. clang a.c -o a

With the -o specified, the AssembleJobAction after OffloadWrapperJobAction
will produce the object file with same name as host code object file.
Due to this clash, the OffloadWrapperAction overwrites the initial host
object file, which results in lld error. This also fixes the multiple definition of __dummy.omp_offloading.entry' issue in D96769 .

Diff Detail

Event Timeline

pdhaliwal created this revision.Feb 23 2021, 5:05 AM
pdhaliwal requested review of this revision.Feb 23 2021, 5:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2021, 5:06 AM
pdhaliwal edited the summary of this revision. (Show Details)Feb 23 2021, 5:48 AM
pdhaliwal edited the summary of this revision. (Show Details)Feb 23 2021, 6:11 AM

Here's a bit of background,
OffloadingPrefix was not getting properly set in the dependent actions of OffloadWrapperJobAction (which are backend [11] and assemble [12]). Since backend [11] and assemble [12] host-wrapper actions have same logic to the other host actions (3 & 4), those will overwrite the previous generated files from host-only actions.

For e.g. following were the names generated for output files previously (marked as bold). (clang -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps -ccc-print-bindings helloworld.c -o helloworld)

  1. "x86_64-unknown-linux-gnu" - "clang", inputs: ["helloworld.c"], output: "helloworld-host-x86_64-unknown-linux-gnu.i"
  2. "x86_64-unknown-linux-gnu" - "clang", inputs: ["helloworld-host-x86_64-unknown-linux-gnu.i"], output: "helloworld-host-x86_64-unknown-linux-gnu.bc"
  3. "x86_64-unknown-linux-gnu" - "clang", inputs: ["helloworld-host-x86_64-unknown-linux-gnu.bc"], output: "helloworld-host-x86_64-unknown-linux-gnu.s"
  4. "x86_64-unknown-linux-gnu" - "clang::as", inputs: ["helloworld-host-x86_64-unknown-linux-gnu.s"], output: "helloworld-host-x86_64-unknown-linux-gnu.o"
  5. "nvptx64-nvidia-cuda" - "clang", inputs: ["helloworld.c"], output: "helloworld-openmp-nvptx64-nvidia-cuda.i"
  6. "nvptx64-nvidia-cuda" - "clang", inputs: ["helloworld-openmp-nvptx64-nvidia-cuda.i", "helloworld-host-x86_64-unknown-linux-gnu.bc"], output: "helloworld-openmp-nvptx64-nvidia-cuda.bc"
  7. "nvptx64-nvidia-cuda" - "clang", inputs: ["helloworld-openmp-nvptx64-nvidia-cuda.bc"], output: "helloworld-openmp-nvptx64-nvidia-cuda.s"
  8. "nvptx64-nvidia-cuda" - "NVPTX::Assembler", inputs: ["helloworld-openmp-nvptx64-nvidia-cuda.s"], output: "helloworld-openmp-nvptx64-nvidia-cuda.o"
  9. "nvptx64-nvidia-cuda" - "NVPTX::OpenMPLinker", inputs: ["helloworld-openmp-nvptx64-nvidia-cuda.o"], output: "a.out-openmp-nvptx64-nvidia-cuda"
  10. "x86_64-unknown-linux-gnu" - "offload wrapper", inputs: ["a.out-openmp-nvptx64-nvidia-cuda"], output: "helloworld-host-x86_64-unknown-linux-gnu-wrapper.bc"
  11. "x86_64-unknown-linux-gnu" - "clang", inputs: ["helloworld-host-x86_64-unknown-linux-gnu-wrapper.bc"], output: "helloworld-host-x86_64-unknown-linux-gnu.s"
  12. "x86_64-unknown-linux-gnu" - "clang::as", inputs: ["helloworld-host-x86_64-unknown-linux-gnu.s"], output: "helloworld-host-x86_64-unknown-linux-gnu.o"
  13. "x86_64-unknown-linux-gnu" - "GNU::Linker", inputs: ["helloworld-host-x86_64-unknown-linux-gnu.o", "helloworld-host-x86_64-unknown-linux-gnu.o"], output: "helloworld"

And here are names generated after this patch applied,

  1. "x86_64-unknown-linux-gnu" - "clang", inputs: ["helloworld.c"], output: "helloworld-host-x86_64-unknown-linux-gnu.i"
  2. "x86_64-unknown-linux-gnu" - "clang", inputs: ["helloworld-host-x86_64-unknown-linux-gnu.i"], output: "helloworld-host-x86_64-unknown-linux-gnu.bc"
  3. "x86_64-unknown-linux-gnu" - "clang", inputs: ["helloworld-host-x86_64-unknown-linux-gnu.bc"], output: "helloworld-host-x86_64-unknown-linux-gnu.s"
  4. "x86_64-unknown-linux-gnu" - "clang::as", inputs: ["helloworld-host-x86_64-unknown-linux-gnu.s"], output: "helloworld-host-x86_64-unknown-linux-gnu.o"
  5. "nvptx64-nvidia-cuda" - "clang", inputs: ["helloworld.c"], output: "helloworld-openmp-nvptx64-nvidia-cuda.i"
  6. "nvptx64-nvidia-cuda" - "clang", inputs: ["helloworld-openmp-nvptx64-nvidia-cuda.i", "helloworld-host-x86_64-unknown-linux-gnu.bc"], output: "helloworld-openmp-nvptx64-nvidia-cuda.bc"
  7. "nvptx64-nvidia-cuda" - "clang", inputs: ["helloworld-openmp-nvptx64-nvidia-cuda.bc"], output: "helloworld-openmp-nvptx64-nvidia-cuda.s"
  8. "nvptx64-nvidia-cuda" - "NVPTX::Assembler", inputs: ["helloworld-openmp-nvptx64-nvidia-cuda.s"], output: "helloworld-openmp-nvptx64-nvidia-cuda.o"
  9. "nvptx64-nvidia-cuda" - "NVPTX::OpenMPLinker", inputs: ["helloworld-openmp-nvptx64-nvidia-cuda.o"], output: "a.out-openmp-nvptx64-nvidia-cuda"
  10. "x86_64-unknown-linux-gnu" - "offload wrapper", inputs: ["a.out-openmp-nvptx64-nvidia-cuda"], output: "helloworld-wrapper-host-x86_64-unknown-linux-gnu.bc"
  11. "x86_64-unknown-linux-gnu" - "clang", inputs: ["helloworld-wrapper-host-x86_64-unknown-linux-gnu.bc"], output: "helloworld-wrapper-host-x86_64-unknown-linux-gnu.s"
  12. "x86_64-unknown-linux-gnu" - "clang::as", inputs: ["helloworld-wrapper-host-x86_64-unknown-linux-gnu.s"], output: "helloworld-wrapper-host-x86_64-unknown-linux-gnu.o"
  13. "x86_64-unknown-linux-gnu" - "GNU::Linker", inputs: ["helloworld-host-x86_64-unknown-linux-gnu.o", "helloworld-wrapper-host-x86_64-unknown-linux-gnu.o"], output: "helloworld"

So for having OffloadingPrefix different for 11 & 12 would require to distinguish latter from 3 & 4 which I don't think is possible. However, the changes to BaseInput in OffloadWrapperJobAction [10] will also reflect in the dependent backend [11] and assemble [12] actions as BaseInput is present in InputInfo of the next actions (line number 4696).

jdoerfert accepted this revision.Feb 24 2021, 9:28 AM

LGTM, assuming it doesn't break support the reasoning makes sense.

This revision is now accepted and ready to land.Feb 24 2021, 9:28 AM
JonChesterfield added a comment.EditedFeb 24 2021, 9:32 AM

Works everywhere we have tried it. Fundamentally it renames a temporary file, so shouldn't break anything. Will be great to have -save-temps working for nvptx.