Prefer std::getenv to ::getenv #108529
Prefer std::getenv to ::getenv #108529
Conversation
Posix call to ::getenv is not guarenteed to be thread safe while C++11 made std::getenv thread safe. This resolves bugs when using llvm in multithreaded environment similar to cms-sw/cmssw#44659
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang-driver Author: Vassil Vassilev (vgvassilev) ChangesPosix call to ::getenv is not guarenteed to be thread safe while C++11 made std::getenv thread safe. This resolves bugs when using llvm in multithreaded environment similar to cms-sw/cmssw#44659 Full diff: https://github.com/llvm/llvm-project/pull/108529.diff 12 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
index a36cb41a63dfb1..8d6590fab281c2 100644
--- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
+++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
@@ -522,7 +522,7 @@ class HTMLLogger : public Logger {
// Nothing interesting here, just subprocess/temp-file plumbing.
llvm::Expected<std::string> renderSVG(llvm::StringRef DotGraph) {
std::string DotPath;
- if (const auto *FromEnv = ::getenv("GRAPHVIZ_DOT"))
+ if (const auto *FromEnv = std::getenv("GRAPHVIZ_DOT"))
DotPath = FromEnv;
else {
auto FromPath = llvm::sys::findProgramByName("dot");
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index e12416e51f8d24..4b65bff86393cd 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1124,7 +1124,7 @@ bool Driver::loadConfigFiles() {
bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
// Disable default config if CLANG_NO_DEFAULT_CONFIG is set to a non-empty
// value.
- if (const char *NoConfigEnv = ::getenv("CLANG_NO_DEFAULT_CONFIG")) {
+ if (const char *NoConfigEnv = std::getenv("CLANG_NO_DEFAULT_CONFIG")) {
if (*NoConfigEnv)
return false;
}
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 320d2901da06ed..8c65c7d2d3d3e5 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -390,7 +390,7 @@ tools::unifyTargetFeatures(ArrayRef<StringRef> Features) {
void tools::addDirectoryList(const ArgList &Args, ArgStringList &CmdArgs,
const char *ArgName, const char *EnvVar) {
- const char *DirList = ::getenv(EnvVar);
+ const char *DirList = std::getenv(EnvVar);
bool CombinedArg = false;
if (!DirList)
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index 2550541a438481..0c7fcd54fb7d21 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -1936,7 +1936,7 @@ getDeploymentTargetFromEnvironmentVariables(const Driver &TheDriver,
static_assert(std::size(EnvVars) == Darwin::LastDarwinPlatform + 1,
"Missing platform");
for (const auto &I : llvm::enumerate(llvm::ArrayRef(EnvVars))) {
- if (char *Env = ::getenv(I.value()))
+ if (char *Env = std::getenv(I.value()))
Targets[I.index()] = Env;
}
@@ -2217,7 +2217,7 @@ void Darwin::AddDeploymentTarget(DerivedArgList &Args) const {
if (!getVFS().exists(A->getValue()))
getDriver().Diag(clang::diag::warn_missing_sysroot) << A->getValue();
} else {
- if (char *env = ::getenv("SDKROOT")) {
+ if (char *env = std::getenv("SDKROOT")) {
// We only use this value as the default if it is an absolute path,
// exists, and it is not the root path.
if (llvm::sys::path::is_absolute(env) && getVFS().exists(env) &&
@@ -3217,13 +3217,13 @@ ToolChain::UnwindTableLevel MachO::getDefaultUnwindTableLevel(const ArgList &Arg
}
bool MachO::UseDwarfDebugFlags() const {
- if (const char *S = ::getenv("RC_DEBUG_OPTIONS"))
+ if (const char *S = std::getenv("RC_DEBUG_OPTIONS"))
return S[0] != '\0';
return false;
}
std::string MachO::GetGlobalDebugPathRemapping() const {
- if (const char *S = ::getenv("RC_DEBUG_PREFIX_MAP"))
+ if (const char *S = std::getenv("RC_DEBUG_PREFIX_MAP"))
return S;
return {};
}
diff --git a/clang/lib/Frontend/PrecompiledPreamble.cpp b/clang/lib/Frontend/PrecompiledPreamble.cpp
index cab5838fceb24d..cf3999341dc2d4 100644
--- a/clang/lib/Frontend/PrecompiledPreamble.cpp
+++ b/clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -203,7 +203,7 @@ class TempPCHFile {
// FIXME: This is a hack so that we can override the preamble file during
// crash-recovery testing, which is the only case where the preamble files
// are not necessarily cleaned up.
- if (const char *TmpFile = ::getenv("CINDEXTEST_PREAMBLE_FILE"))
+ if (const char *TmpFile = std::getenv("CINDEXTEST_PREAMBLE_FILE"))
return std::unique_ptr<TempPCHFile>(new TempPCHFile(TmpFile));
llvm::SmallString<128> File;
diff --git a/clang/tools/clang-installapi/Options.cpp b/clang/tools/clang-installapi/Options.cpp
index 1ca1d583d5ccdf..f977275357830d 100644
--- a/clang/tools/clang-installapi/Options.cpp
+++ b/clang/tools/clang-installapi/Options.cpp
@@ -455,10 +455,10 @@ bool Options::processLinkerOptions(InputArgList &Args) {
drv::OPT_fapplication_extension, drv::OPT_fno_application_extension,
/*Default=*/LinkerOpts.AppExtensionSafe);
- if (::getenv("LD_NO_ENCRYPT") != nullptr)
+ if (std::getenv("LD_NO_ENCRYPT") != nullptr)
LinkerOpts.AppExtensionSafe = true;
- if (::getenv("LD_APPLICATION_EXTENSION_SAFE") != nullptr)
+ if (std::getenv("LD_APPLICATION_EXTENSION_SAFE") != nullptr)
LinkerOpts.AppExtensionSafe = true;
// Capture library paths.
@@ -511,7 +511,7 @@ bool Options::processFrontendOptions(InputArgList &Args) {
} else if (FEOpts.ISysroot.empty()) {
// Mirror CLANG and obtain the isysroot from the SDKROOT environment
// variable, if it wasn't defined by the command line.
- if (auto *Env = ::getenv("SDKROOT")) {
+ if (auto *Env = std::getenv("SDKROOT")) {
if (StringRef(Env) != "/" && llvm::sys::path::is_absolute(Env) &&
FM->getOptionalFileRef(Env))
FEOpts.ISysroot = Env;
diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index 83b5bbb71f5212..4a7a72e0e539c9 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -121,12 +121,12 @@ static void getCLEnvVarOptions(std::string &EnvValue, llvm::StringSaver &Saver,
template <class T>
static T checkEnvVar(const char *EnvOptSet, const char *EnvOptFile,
std::string &OptFile) {
- const char *Str = ::getenv(EnvOptSet);
+ const char *Str = std::getenv(EnvOptSet);
if (!Str)
return T{};
T OptVal = Str;
- if (const char *Var = ::getenv(EnvOptFile))
+ if (const char *Var = std::getenv(EnvOptFile))
OptFile = Var;
return OptVal;
}
@@ -152,7 +152,7 @@ static bool SetBackdoorDriverOutputsFromEnvVars(Driver &TheDriver) {
return false;
}
- const char *FilteringStr = ::getenv("CC_PRINT_HEADERS_FILTERING");
+ const char *FilteringStr = std::getenv("CC_PRINT_HEADERS_FILTERING");
HeaderIncludeFilteringKind Filtering;
if (!stringToHeaderIncludeFiltering(FilteringStr, Filtering)) {
TheDriver.Diag(clang::diag::err_drv_print_header_env_var)
@@ -294,7 +294,7 @@ int clang_main(int Argc, char **Argv, const llvm::ToolContext &ToolContext) {
llvm::StringSet<> SavedStrings;
// Handle CCC_OVERRIDE_OPTIONS, used for editing a command line behind the
// scenes.
- if (const char *OverrideStr = ::getenv("CCC_OVERRIDE_OPTIONS")) {
+ if (const char *OverrideStr = std::getenv("CCC_OVERRIDE_OPTIONS")) {
// FIXME: Driver shouldn't take extra initial argument.
driver::applyOverrideOptions(Args, OverrideStr, SavedStrings,
&llvm::errs());
@@ -376,7 +376,7 @@ int clang_main(int Argc, char **Argv, const llvm::ToolContext &ToolContext) {
}
ReproLevel = *Level;
}
- if (!!::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH"))
+ if (!!std::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH"))
ReproLevel = Driver::ReproLevel::Always;
int Res = 1;
@@ -420,7 +420,7 @@ int clang_main(int Argc, char **Argv, const llvm::ToolContext &ToolContext) {
// Print the bug report message that would be printed if we did actually
// crash, but only if we're crashing due to FORCE_CLANG_DIAGNOSTICS_CRASH.
- if (::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH"))
+ if (std::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH"))
llvm::dbgs() << llvm::getBugReportMsg();
if (FailingCommand != nullptr &&
TheDriver.maybeGenerateCompilationDiagnostics(CommandStatus, ReproLevel,
diff --git a/clang/tools/libclang/ARCMigrate.cpp b/clang/tools/libclang/ARCMigrate.cpp
index da8a7e4b9130b9..befaa7d98ec4be 100644
--- a/clang/tools/libclang/ARCMigrate.cpp
+++ b/clang/tools/libclang/ARCMigrate.cpp
@@ -37,7 +37,7 @@ CXRemapping clang_getRemappings(const char *migrate_dir_path) {
llvm::errs() << "error: feature not enabled in this build\n";
return nullptr;
#else
- bool Logging = ::getenv("LIBCLANG_LOGGING");
+ bool Logging = std::getenv("LIBCLANG_LOGGING");
if (!migrate_dir_path) {
if (Logging)
@@ -80,7 +80,7 @@ CXRemapping clang_getRemappingsFromFileList(const char **filePaths,
llvm::errs() << "error: feature not enabled in this build\n";
return nullptr;
#else
- bool Logging = ::getenv("LIBCLANG_LOGGING");
+ bool Logging = std::getenv("LIBCLANG_LOGGING");
std::unique_ptr<Remap> remap(new Remap());
diff --git a/clang/tools/libclang/CLog.h b/clang/tools/libclang/CLog.h
index 6ce43a01ee8f2d..aeac62fe0dacff 100644
--- a/clang/tools/libclang/CLog.h
+++ b/clang/tools/libclang/CLog.h
@@ -43,7 +43,7 @@ class Logger : public RefCountedBase<Logger> {
llvm::raw_svector_ostream LogOS;
public:
static const char *getEnvVar() {
- static const char *sCachedVar = ::getenv("LIBCLANG_LOGGING");
+ static const char *sCachedVar = std::getenv("LIBCLANG_LOGGING");
return sCachedVar;
}
static bool isLoggingEnabled() { return getEnvVar() != nullptr; }
diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index cf05db546e0214..3f29f966bf5f0c 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -369,7 +369,7 @@ ErrorOr<space_info> disk_space(const Twine &Path) {
std::error_code current_path(SmallVectorImpl<char> &result) {
result.clear();
- const char *pwd = ::getenv("PWD");
+ const char *pwd = std::getenv("PWD");
llvm::sys::fs::file_status PWDStatus, DotStatus;
if (pwd && llvm::sys::path::is_absolute(pwd) &&
!llvm::sys::fs::status(pwd, PWDStatus) &&
diff --git a/llvm/lib/Support/Unix/Process.inc b/llvm/lib/Support/Unix/Process.inc
index 84b10ff5d1d08a..f24972f541f0f3 100644
--- a/llvm/lib/Support/Unix/Process.inc
+++ b/llvm/lib/Support/Unix/Process.inc
@@ -199,7 +199,7 @@ void Process::PreventCoreFiles() {
std::optional<std::string> Process::GetEnv(StringRef Name) {
std::string NameStr = Name.str();
- const char *Val = ::getenv(NameStr.c_str());
+ const char *Val = std::getenv(NameStr.c_str());
if (!Val)
return std::nullopt;
return std::string(Val);
diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp
index 463a6991dac515..24b18865d03b0e 100644
--- a/llvm/unittests/Support/Path.cpp
+++ b/llvm/unittests/Support/Path.cpp
@@ -459,7 +459,7 @@ class WithEnv {
public:
WithEnv(const char *Var, const char *Value) : Var(Var) {
- if (const char *V = ::getenv(Var))
+ if (const char *V = std::getenv(Var))
OriginalValue.emplace(V);
if (Value)
::setenv(Var, Value, 1);
@@ -480,7 +480,7 @@ TEST(Support, HomeDirectory) {
#ifdef _WIN32
expected = getEnvWin(L"USERPROFILE");
#else
- if (char const *path = ::getenv("HOME"))
+ if (char const *path = std::getenv("HOME"))
expected = path;
#endif
// Do not try to test it if we don't know what to expect.
|
There was a problem hiding this comment.
Are we sure this is going to do what we think?
llvm-project/libcxx/include/cstdlib
Line 127 in a41bb71
https://github.com/gcc-mirror/gcc/blob/494d3c3faaee0dbde696ea334f8e242ae85ae2b5/libstdc%2B%2B-v3/include/c_compatibility/stdlib.h#L65
I am not sure if I understand the code you pointed to. That's one of the reasons I did not want to expand that patch beyond clang and llvm. |
Both libc++ and libstdc++ (which target POSIX systems) seem to have no code that provides those thread-safety guarantees; they're exposing CC @ldionne @philnik777 for more opinions |
The issue I linked in the PR description empirically seems to prove that |
|
If the implementation is conforming this doesn't do anything. In fact, they have to be the same function. |
That was my understanding as well, thanks! So it sounds like the problem is that there's an underlying C23 7.24.4.6p2: ... The getenv function is not required to avoid data races with other threads of execution that modify the environment list. POSIX: ... Some earlier versions of this standard did not require getenv() to be thread-safe because it was allowed to return a value pointing to an internal buffer. However, this behavior allowed by the ISO C standard is no longer allowed by POSIX.1. POSIX.1 requires the environment data to be available through environ[], so there is no reason why getenv() can't return a pointer to the actual data instead of a copy. Therefore getenv() is now required to be thread-safe (except when another thread modifies the environment). ... Curiously, it seems like |
|
Okay, thanks for clarifying. Shall I close this one? |
I don't see a lot of value in the change, but because it's an NFC and the changes only impact code always compiled as C++ anyway, it's not entirely unreasonable. Personally, I'd close, but it's up to you (if you move ahead with it as an NFC change, be sure to reword the commit title and summary accordingly though). |
Posix call to ::getenv is not guarenteed to be thread safe while C++11 made std::getenv thread safe.
This resolves bugs when using llvm in multithreaded environment similar to cms-sw/cmssw#44659