From f96c4222a05088a035b88ee0535057b05d6c7bc3 Mon Sep 17 00:00:00 2001 From: Artem Gindinson Date: Fri, 3 Apr 2020 11:36:06 +0300 Subject: [PATCH 1/2] [SYCL][Driver] Enforce unique filenames when -save-temps is used With the introduction of file table transformations, multiple intermediate files with the same extension are now occurring within any SYCL compilation (namely .txt and .table files). Unless we treat this during the static creation of comands, filename collision is bound to occur when saving temps. This breaks -save-temps compilations due to files being overwritten. This commit ensures that unique files are created upon a potential collision in the presence of -save-temps. Signed-off-by: Artem Gindinson --- clang/include/clang/Driver/Driver.h | 5 ++ clang/lib/Driver/Driver.cpp | 57 +++++++++++++++++------ clang/test/Driver/sycl-offload.c | 14 +++--- sycl/test/regression/fsycl-save-temps.cpp | 23 +++++++++ 4 files changed, 78 insertions(+), 21 deletions(-) create mode 100644 sycl/test/regression/fsycl-save-temps.cpp diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h index 6679fae41f436..67aabdab0e8a5 100644 --- a/clang/include/clang/Driver/Driver.h +++ b/clang/include/clang/Driver/Driver.h @@ -540,6 +540,11 @@ class Driver { /// GCC goes to extra lengths here to be a bit more robust. std::string GetTemporaryPath(StringRef Prefix, StringRef Suffix) const; + /// GetUniquePath = Return the pathname of a unique file to use + /// as part of compilation. The file will have the given base name (BaseName) + /// and extension (Ext). + std::string GetUniquePath(StringRef BaseName, StringRef Ext) const; + /// GetTemporaryDirectory - Return the pathname of a temporary directory to /// use as part of compilation; the directory will have the given prefix. std::string GetTemporaryDirectory(StringRef Prefix) const; diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp index f060024155f4a..19c509a8613cf 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -6071,21 +6071,38 @@ const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA, NamedOutput = C.getArgs().MakeArgString(TempPath.c_str()); } - // If we're saving temps and the temp file conflicts with the input file, - // then avoid overwriting input file. - if (!AtTopLevel && isSaveTempsEnabled() && NamedOutput == BaseName) { - bool SameFile = false; - SmallString<256> Result; - llvm::sys::fs::current_path(Result); - llvm::sys::path::append(Result, BaseName); - llvm::sys::fs::equivalent(BaseInput, Result.c_str(), SameFile); - // Must share the same path to conflict. - if (SameFile) { - StringRef Name = llvm::sys::path::filename(BaseInput); - std::pair Split = Name.split('.'); - std::string TmpName = GetTemporaryPath( + if (isSaveTempsEnabled()) { + // If we're saving temps and the temp file conflicts with any + // input/resulting file, then avoid overwriting. + if (!AtTopLevel) { + bool SameFile = false; + SmallString<256> Result; + llvm::sys::fs::current_path(Result); + llvm::sys::path::append(Result, BaseName); + llvm::sys::fs::equivalent(BaseInput, Result.c_str(), SameFile); + // Must share the same path to conflict. + if (SameFile) { + StringRef Name = llvm::sys::path::filename(BaseInput); + std::pair Split = Name.split('.'); + std::string TmpName = GetTemporaryPath( + Split.first, types::getTypeTempSuffix(JA.getType(), IsCLMode())); + return C.addTempFile(C.getArgs().MakeArgString(TmpName)); + } + } + + const auto &ResultFiles = C.getResultFiles(); + const auto CollidingFilenameIt = + llvm::find_if(ResultFiles, [NamedOutput](const auto &It) { + return StringRef(NamedOutput).equals(It.second); + }); + if (CollidingFilenameIt != ResultFiles.end()) { + // Upon any collision, a unique hash will be appended to the filename, + // similar to what is done for temporary files in the regular flow. + StringRef CollidingName(CollidingFilenameIt->second); + std::pair Split = CollidingName.split('.'); + std::string UniqueName = GetUniquePath( Split.first, types::getTypeTempSuffix(JA.getType(), IsCLMode())); - return C.addTempFile(C.getArgs().MakeArgString(TmpName)); + return C.addResultFile(C.getArgs().MakeArgString(UniqueName), &JA); } } @@ -6215,6 +6232,18 @@ std::string Driver::GetTemporaryPath(StringRef Prefix, StringRef Suffix) const { return std::string(Path.str()); } +std::string Driver::GetUniquePath(StringRef BaseName, StringRef Ext) const { + SmallString<128> Path; + std::error_code EC = llvm::sys::fs::createUniqueFile( + Twine(BaseName) + Twine("-%%%%%%.") + Ext, Path); + if (EC) { + Diag(clang::diag::err_unable_to_make_temp) << EC.message(); + return ""; + } + + return std::string(Path.str()); +} + std::string Driver::GetTemporaryDirectory(StringRef Prefix) const { SmallString<128> Path; std::error_code EC = llvm::sys::fs::createUniqueDirectory(Prefix, Path); diff --git a/clang/test/Driver/sycl-offload.c b/clang/test/Driver/sycl-offload.c index 5609870235283..f26821dcb6e0a 100644 --- a/clang/test/Driver/sycl-offload.c +++ b/clang/test/Driver/sycl-offload.c @@ -752,19 +752,19 @@ // RUN: | FileCheck %s --check-prefix=CHK-FSYCL-SAVE-TEMPS // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-fsycl-is-device"{{.*}} "-o" "[[DEVICE_BASE_NAME:[a-z0-9-]+]].ii" // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-fsycl-is-device"{{.*}} "-o" "[[DEVICE_BASE_NAME]].bc"{{.*}} "[[DEVICE_BASE_NAME]].ii" -// CHK-FSYCL-SAVE-TEMPS: llvm-link{{.*}} "[[DEVICE_BASE_NAME]].bc"{{.*}} "-o" "[[DEVICE_BASE_NAME]].bc" -// CHK-FSYCL-SAVE-TEMPS: sycl-post-link{{.*}} "-o" "[[DEVICE_BASE_NAME]].table" "[[DEVICE_BASE_NAME]].bc" +// CHK-FSYCL-SAVE-TEMPS: llvm-link{{.*}} "[[DEVICE_BASE_NAME]].bc"{{.*}} "-o" "[[LINKED_DEVICE_BC:.*\.bc]]" +// CHK-FSYCL-SAVE-TEMPS: sycl-post-link{{.*}} "-o" "[[DEVICE_BASE_NAME]].table" "[[LINKED_DEVICE_BC]]" // CHK-FSYCL-SAVE-TEMPS: file-table-tform{{.*}} "-o" "[[DEVICE_BASE_NAME]].txt" "[[DEVICE_BASE_NAME]].table" -// CHK-FSYCL-SAVE-TEMPS: llvm-foreach{{.*}} "-o" "[[DEVICE_BASE_NAME]].txt" {{.*}} "[[DEVICE_BASE_NAME]].txt" -// CHK-FSYCL-SAVE-TEMPS: file-table-tform{{.*}} "-o" "[[DEVICE_BASE_NAME]].table" "[[DEVICE_BASE_NAME]].table" "[[DEVICE_BASE_NAME]].txt" -// CHK-FSYCL-SAVE-TEMPS: clang-offload-wrapper{{.*}} "-o=[[TEMPFILE:[a-z0-9_/-]+]].bc"{{.*}} "-batch" "[[DEVICE_BASE_NAME]].table" -// CHK-FSYCL-SAVE-TEMPS: llc{{.*}} "-o" "[[DEVICE_OBJ_NAME:[a-z0-9\-]+]].o"{{.*}} "[[TEMPFILE]].bc" +// CHK-FSYCL-SAVE-TEMPS: llvm-foreach{{.*}}llvm-spirv{{.*}} "-o" "[[SPIRV_FILE_LIST:.*\.txt]]" {{.*}}"[[DEVICE_BASE_NAME]].txt" +// CHK-FSYCL-SAVE-TEMPS: file-table-tform{{.*}} "-o" "[[PRE_WRAPPER_TABLE:.*\.table]]" "[[DEVICE_BASE_NAME]].table" "[[SPIRV_FILE_LIST]]" +// CHK-FSYCL-SAVE-TEMPS: clang-offload-wrapper{{.*}} "-o=[[WRAPPER_TEMPFILE_NAME:[a-z0-9_/-]+]].bc"{{.*}} "-batch" "[[PRE_WRAPPER_TABLE]]" +// CHK-FSYCL-SAVE-TEMPS: llc{{.*}} "-o" "[[DEVICE_OBJ_NAME:.*\.o]]"{{.*}} "[[WRAPPER_TEMPFILE_NAME]].bc" // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-fsycl-is-device"{{.*}} "-fsycl-int-header=[[DEVICE_BASE_NAME]].h"{{.*}} "[[DEVICE_BASE_NAME]].ii" // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-include" "[[DEVICE_BASE_NAME]].h"{{.*}} "-fsycl-is-host"{{.*}} "-o" "[[HOST_BASE_NAME:[a-z0-9_-]+]].ii" // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-o" "[[HOST_BASE_NAME:.*]].bc"{{.*}} "[[HOST_BASE_NAME]].ii" // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-o" "[[HOST_BASE_NAME:.*]].s"{{.*}} "[[HOST_BASE_NAME]].bc" // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-o" "[[HOST_BASE_NAME:.*]].o"{{.*}} "[[HOST_BASE_NAME]].s" -// CHK-FSYCL-SAVE-TEMPS: ld{{.*}} "[[HOST_BASE_NAME]].o"{{.*}} "[[DEVICE_OBJ_NAME]].o" +// CHK-FSYCL-SAVE-TEMPS: ld{{.*}} "[[HOST_BASE_NAME]].o"{{.*}} "[[DEVICE_OBJ_NAME]]" /// -fsycl with /Fo testing // RUN: %clang_cl -fsycl /Fosomefile.obj -c %s -### 2>&1 \ diff --git a/sycl/test/regression/fsycl-save-temps.cpp b/sycl/test/regression/fsycl-save-temps.cpp new file mode 100644 index 0000000000000..a6305fb1aecfe --- /dev/null +++ b/sycl/test/regression/fsycl-save-temps.cpp @@ -0,0 +1,23 @@ +//==--------------- fsycl-save-temps.cpp -----------------------------------==// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// Verify that a sample compilation succeeds with -save-temps +// RUN: %clangxx -fsycl -save-temps %s -o %t.out +// RUN: %CPU_RUN_PLACEHOLDER %t.out + +#include + +void foo() {} + +int main() { + cl::sycl::queue Q; + Q.submit([](cl::sycl::handler &Cgh) { + Cgh.single_task([]() { foo(); }); + }); + return 0; +} From 67e9559bbacb1af8995c59ee21e04b5dcdc367ba Mon Sep 17 00:00:00 2001 From: Artem Gindinson Date: Mon, 20 Apr 2020 13:59:09 +0300 Subject: [PATCH 2/2] Address review comments: - Do not execute the E2E test - Add checks for conflicts - XFAIL the E2E test on Windows Signed-off-by: Artem Gindinson --- clang/test/Driver/sycl-offload.c | 5 ++++- sycl/test/regression/fsycl-save-temps.cpp | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/clang/test/Driver/sycl-offload.c b/clang/test/Driver/sycl-offload.c index f26821dcb6e0a..d4d4db93f1c79 100644 --- a/clang/test/Driver/sycl-offload.c +++ b/clang/test/Driver/sycl-offload.c @@ -749,14 +749,17 @@ // RUN: %clang -fsycl -target x86_64-unknown-linux-gnu -save-temps %s -### 2>&1 // RUN: %clang -fsycl -fsycl-targets=spir64-unknown-unknown-sycldevice -target x86_64-unknown-linux-gnu -save-temps %s -### 2>&1 // RUN: %clangxx -fsycl -fsycl-targets=spir64-unknown-unknown-sycldevice -target x86_64-unknown-linux-gnu -save-temps %s -### 2>&1 \ -// RUN: | FileCheck %s --check-prefix=CHK-FSYCL-SAVE-TEMPS +// RUN: | FileCheck %s --check-prefixes=CHK-FSYCL-SAVE-TEMPS,CHK-FSYCL-SAVE-TEMPS-CONFL // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-fsycl-is-device"{{.*}} "-o" "[[DEVICE_BASE_NAME:[a-z0-9-]+]].ii" // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-fsycl-is-device"{{.*}} "-o" "[[DEVICE_BASE_NAME]].bc"{{.*}} "[[DEVICE_BASE_NAME]].ii" // CHK-FSYCL-SAVE-TEMPS: llvm-link{{.*}} "[[DEVICE_BASE_NAME]].bc"{{.*}} "-o" "[[LINKED_DEVICE_BC:.*\.bc]]" +// CHK-FSYCL-SAVE-TEMPS-CONFL-NOT: "[[DEVICE_BASE_NAME]].bc"{{.*}} "[[DEVICE_BASE_NAME]].bc" // CHK-FSYCL-SAVE-TEMPS: sycl-post-link{{.*}} "-o" "[[DEVICE_BASE_NAME]].table" "[[LINKED_DEVICE_BC]]" // CHK-FSYCL-SAVE-TEMPS: file-table-tform{{.*}} "-o" "[[DEVICE_BASE_NAME]].txt" "[[DEVICE_BASE_NAME]].table" // CHK-FSYCL-SAVE-TEMPS: llvm-foreach{{.*}}llvm-spirv{{.*}} "-o" "[[SPIRV_FILE_LIST:.*\.txt]]" {{.*}}"[[DEVICE_BASE_NAME]].txt" +// CHK-FSYCL-SAVE-TEMPS-CONFL-NOT: "-o" "[[DEVICE_BASE_NAME]].txt" {{.*}}"[[DEVICE_BASE_NAME]].txt" // CHK-FSYCL-SAVE-TEMPS: file-table-tform{{.*}} "-o" "[[PRE_WRAPPER_TABLE:.*\.table]]" "[[DEVICE_BASE_NAME]].table" "[[SPIRV_FILE_LIST]]" +// CHK-FSYCL-SAVE-TEMPS-CONFL-NOT: "-o" "[[DEVICE_BASE_NAME]].table"{{.*}} "[[DEVICE_BASE_NAME]].table" // CHK-FSYCL-SAVE-TEMPS: clang-offload-wrapper{{.*}} "-o=[[WRAPPER_TEMPFILE_NAME:[a-z0-9_/-]+]].bc"{{.*}} "-batch" "[[PRE_WRAPPER_TABLE]]" // CHK-FSYCL-SAVE-TEMPS: llc{{.*}} "-o" "[[DEVICE_OBJ_NAME:.*\.o]]"{{.*}} "[[WRAPPER_TEMPFILE_NAME]].bc" // CHK-FSYCL-SAVE-TEMPS: clang{{.*}} "-fsycl-is-device"{{.*}} "-fsycl-int-header=[[DEVICE_BASE_NAME]].h"{{.*}} "[[DEVICE_BASE_NAME]].ii" diff --git a/sycl/test/regression/fsycl-save-temps.cpp b/sycl/test/regression/fsycl-save-temps.cpp index a6305fb1aecfe..7eb502f3b4418 100644 --- a/sycl/test/regression/fsycl-save-temps.cpp +++ b/sycl/test/regression/fsycl-save-temps.cpp @@ -8,7 +8,6 @@ // Verify that a sample compilation succeeds with -save-temps // RUN: %clangxx -fsycl -save-temps %s -o %t.out -// RUN: %CPU_RUN_PLACEHOLDER %t.out #include @@ -21,3 +20,6 @@ int main() { }); return 0; } + +// TODO: Address a Windows-specific issue with integration header filenames +// XFAIL: system-windows