Skip to content

Conversation

perry-ca
Copy link
Contributor

PR #111976 was enabling the tests updated in the PR to run on all systems. We found a few didn't run on z/OS. I tracked the problem down to:

  1. the ExecuteToolChainProgram() function wasn't passing the executable name as the first arg. That was causing exec on z/OS to fail.
  2. the temp file needs to be a text file so codepage conversion happens.

@perry-ca perry-ca self-assigned this Oct 29, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Oct 29, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 29, 2024

@llvm/pr-subscribers-clang-driver

Author: Sean Perry (perry-ca)

Changes

PR #111976 was enabling the tests updated in the PR to run on all systems. We found a few didn't run on z/OS. I tracked the problem down to:

  1. the ExecuteToolChainProgram() function wasn't passing the executable name as the first arg. That was causing exec on z/OS to fail.
  2. the temp file needs to be a text file so codepage conversion happens.

Full diff: https://github.com/llvm/llvm-project/pull/114067.diff

1 Files Affected:

  • (modified) clang/lib/Driver/ToolChain.cpp (+4-2)
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 34de0043ca012a..bdf3da0c96adca 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -109,7 +109,8 @@ ToolChain::ToolChain(const Driver &D, const llvm::Triple &T,
 llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>>
 ToolChain::executeToolChainProgram(StringRef Executable) const {
   llvm::SmallString<64> OutputFile;
-  llvm::sys::fs::createTemporaryFile("toolchain-program", "txt", OutputFile);
+  llvm::sys::fs::createTemporaryFile("toolchain-program", "txt", OutputFile,
+                                     llvm::sys::fs::OF_Text);
   llvm::FileRemover OutputRemover(OutputFile.c_str());
   std::optional<llvm::StringRef> Redirects[] = {
       {""},
@@ -128,7 +129,8 @@ ToolChain::executeToolChainProgram(StringRef Executable) const {
                                          *Str + "'");
     SecondsToWait = std::max(SecondsToWait, 0); // infinite
   }
-  if (llvm::sys::ExecuteAndWait(Executable, {}, {}, Redirects, SecondsToWait,
+  if (llvm::sys::ExecuteAndWait(Executable, {Executable}, {}, Redirects,
+                                SecondsToWait,
                                 /*MemoryLimit=*/0, &ErrorMessage))
     return llvm::createStringError(std::error_code(),
                                    Executable + ": " + ErrorMessage);

@llvmbot
Copy link
Member

llvmbot commented Oct 29, 2024

@llvm/pr-subscribers-clang

Author: Sean Perry (perry-ca)

Changes

PR #111976 was enabling the tests updated in the PR to run on all systems. We found a few didn't run on z/OS. I tracked the problem down to:

  1. the ExecuteToolChainProgram() function wasn't passing the executable name as the first arg. That was causing exec on z/OS to fail.
  2. the temp file needs to be a text file so codepage conversion happens.

Full diff: https://github.com/llvm/llvm-project/pull/114067.diff

1 Files Affected:

  • (modified) clang/lib/Driver/ToolChain.cpp (+4-2)
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 34de0043ca012a..bdf3da0c96adca 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -109,7 +109,8 @@ ToolChain::ToolChain(const Driver &D, const llvm::Triple &T,
 llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>>
 ToolChain::executeToolChainProgram(StringRef Executable) const {
   llvm::SmallString<64> OutputFile;
-  llvm::sys::fs::createTemporaryFile("toolchain-program", "txt", OutputFile);
+  llvm::sys::fs::createTemporaryFile("toolchain-program", "txt", OutputFile,
+                                     llvm::sys::fs::OF_Text);
   llvm::FileRemover OutputRemover(OutputFile.c_str());
   std::optional<llvm::StringRef> Redirects[] = {
       {""},
@@ -128,7 +129,8 @@ ToolChain::executeToolChainProgram(StringRef Executable) const {
                                          *Str + "'");
     SecondsToWait = std::max(SecondsToWait, 0); // infinite
   }
-  if (llvm::sys::ExecuteAndWait(Executable, {}, {}, Redirects, SecondsToWait,
+  if (llvm::sys::ExecuteAndWait(Executable, {Executable}, {}, Redirects,
+                                SecondsToWait,
                                 /*MemoryLimit=*/0, &ErrorMessage))
     return llvm::createStringError(std::error_code(),
                                    Executable + ": " + ErrorMessage);

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fix commit description

@perry-ca perry-ca changed the title need to pass executable name as arg[0] Pass the executable name as arg[0] when calling ExecuteAndWait() Oct 29, 2024
@perry-ca perry-ca merged commit 5545f76 into llvm:main Oct 30, 2024
11 checks passed
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…m#114067)

PR llvm#111976 was enabling the
tests updated in the PR to run on all systems. We found a few didn't run
on z/OS. I tracked the problem down to:
1. the ExecuteToolChainProgram() function wasn't passing the executable
name as the first arg. That was causing exec on z/OS to fail.
2. the temp file needs to be a text file so codepage conversion happens.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants