2

I have a project with tight level of clang-tidy static analysis.

In order to check some consistencies in the environment I am probing the value of a few environment variables.

int main() {
    char const* ompi_size_cstr = std::getenv("OMPI_COMM_WORLD_SIZE");
    ...
}

(nevermind the meaning of the variable).

But now I get this warning:

/builds/user/mpi3/environment.hpp:49:51: error: function is not thread safe [concurrency-mt-unsafe,-warnings-as-errors]
        const char* ompi_size_cstr = std::getenv("OMPI_COMM_WORLD_SIZE");

First, if I look at the documentation, https://en.cppreference.com/w/cpp/utility/program/getenv, it says that the function is thread-safe since C++11 (I am using C++17). But maybe the warning is referring to the fact that the environment variable itself could change behind the scenes.

Second, even if this is not thread-safe in some sense: What can I do about it? Is there a canonical solution to use getenv?,

for example, by locking a manually introduced get_env_mtx mutex, or perhaps making my variable declaration static? As in,

   static const char* ompi_size_cstr = std::getenv("OMPI_COMM_WORLD_SIZE");

or

   static std::string ompi_size_cstr = std::getenv("OMPI_COMM_WORLD_SIZE");

Of course, I can also introduce an exception to the rule, // NOLINT(concurrency-mt-unsafe) but I wanted to know if there is a fix that I can express as better code instead. It would be also good to improve the code even if at the end I have to add a NOLINT anyway.

Note: I am using clang-tidy version 14.0.6, and in C++17.

alfC
  • 14,261
  • 4
  • 67
  • 118
  • 1
    "I have a project with tight level of clang-tidy static analysis." - Good. You'll have fewer bugs. – Jesper Juhl Nov 17 '22 at 18:39
  • 1
    @JesperJuhl I agree, it is a personal project and I imposed that on myself. I learned a lot by activating all the warnings I possibly could. – alfC Nov 17 '22 at 18:41
  • @RichardCritten, yes, I am doing that. In reality the code is not in main but is in a function (called `mpi_initialize`) called immediately after main which I think it counts as "before any threads". But perhaps clang-tidy cannot see through that. Maybe there is a way to annotate the function as "called once" or "called only from main thread". – alfC Nov 17 '22 at 18:49
  • @JesperJuhl, in case you are curious, https://gitlab.com/correaa/boost-mpi3/-/blob/master/.clang-tidy . As you see almost all analysis is active, only few disabled rules. – alfC Nov 17 '22 at 18:50
  • [std::call_once](https://en.cppreference.com/w/cpp/thread/call_once)? Probably not, but who knows.. – Jesper Juhl Nov 17 '22 at 18:50
  • Maybe submit a bug report to the LLVM team? – Quimby Nov 17 '22 at 18:52
  • @Quimby, yes, if I see that the test fails with the example in the very https://en.cppreference.com/w/cpp/utility/program/getenv (see at the bottom of the page) or an equivalent situation I will submit a bug to LLVM. – alfC Nov 17 '22 at 18:54
  • "Environment variables" are necessarily an *external* resource. Maybe the "thread safe" warning in this case is a red herring ...? – Mike Robinson Nov 17 '22 at 18:59
  • 1
    the cppreference actually says "This function is thread-safe *as long as no other function modifies the host environment.*" I don't think this can even be checked. which make it effectively not thread-safe from static analyzer's point. – apple apple Nov 17 '22 at 19:02
  • @appleapple, ok so it is not thread safe in the same sense that reading from a file is not safe. Another *process* could be touching that "external" resource; not only another thread (which can be ruled out statically, e.g., when running from main). Perhaps clang-tidy is trying to protect me from that. It there should be another name for that, like "process-safe" or "environment-safe", it has little to with threads really. – alfC Nov 18 '22 at 08:18

1 Answers1

1

It is probably an outdated warning or some confusion with C's getenv which is not thread-safe at all.

There's getenv_s (comes with C11) that stores the environment variable locally. You can use that instead.

ALX23z
  • 4,456
  • 1
  • 11
  • 18
  • Thanks, for the pointer. `getenv_s` at least is explicit in the sense that it makes a copy of string from the environment instead of returning a pointer to the environment. Seems more complicated to use. Microsoft seems to provide a templated (in size) version of the function. https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-s-wgetenv-s?view=msvc-170 – alfC Nov 17 '22 at 22:13