[lldb] Prevent race condition when fetching /proc/cpuinfo

[wallace]

Summary

@clayborg found a potential race condition when setting a static
variable. The fix seems simply to use call_once.

All relevant tests pass.

[labath]

lldb/source/Plugins/Process/Linux/Procfs.cpp

19–20

How about storing this as ErrorOr<std::vector<uint8_t>> (that's what getProcFile returns anyway), and initializing it via an immediately-evaluated lambda ?
I.e., something like

auto cpu_info = [] -> ErrorOr<std::vector<uint8_t>> {
  if (auto buffer_or_error = getProcFile("cpuinfo")) {
    return std::vector<uint8_t>(...);
  } else {
    return buffer_or_error.getError();
  }
}();

[wallace]

use an idea similar to labath's to simplify this code while still using call_once

[clayborg]

lgtm. Pavel?

[labath]

It's not clear to me why you're insisting on call_once, when c++ guarantees that the function-local statics will only be initialized once (atomically). And with the new version, we don't even need the lambda..

lldb/source/Plugins/Process/Linux/Procfs.cpp

11

btw, llvm does not generally put blank lines between include headers. omitting those lets clang format reorder everything according to the official style.

20–28

And why not this?

28

lol

[wallace]

lldb/source/Plugins/Process/Linux/Procfs.cpp

11

oh, i didn't know that clang-format ensures that style guideline. Thanks!

20–28

i didn't know that local static initializations are ensured by the compiler not to have race conditions. TIL
I'll just do what you are suggesting here

[wallace]

simplify this diff following @labath's advice

[labath]

lldb/source/Plugins/Process/Linux/Procfs.cpp

27–29

return arrayRefFromStringRef(buffer.getBuffer());

[clayborg]

In D131081#3699295, @labath wrote:

It's not clear to me why you're insisting on call_once, when c++ guarantees that the function-local statics will only be initialized once (atomically). And with the new version, we don't even need the lambda..

I tend to use call_once to avoid an issue, IIRC, that older windows compilers had with the "statics will only be initialized once (atomically)". I seem to remember Microsoft compilers were not fully compliant with this and that it could cause multi-threading issues, but I don't know if this has been fixed or if this wouldn't be an issue in this case.

[wallace]

That's important to know. At least in this case this code only runs on Linux, so hopefully we are good with the atomic static initialization.

[clayborg]

In D131081#3707956, @wallace wrote:

That's important to know. At least in this case this code only runs on Linux, so hopefully we are good with the atomic static initialization.

Good point! Should be good to go

lldb/source/Plugins/Process/Linux/Procfs.cpp

 //===-- Procfs.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
 //
 //===----------------------------------------------------------------------===//
 
 #include "Procfs.h"
-
 #include "lldb/Host/linux/Support.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Threading.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace process_linux;
 using namespace llvm;
 
 Expected<ArrayRef<uint8_t>> lldb_private::process_linux::GetProcfsCpuInfo() {
-  static Optional<std::vector<uint8_t>> cpu_info;
-  if (!cpu_info) {
-    auto buffer_or_error = errorOrToExpected(getProcFile("cpuinfo"));
-    if (!buffer_or_error)
-      return buffer_or_error.takeError();
-    MemoryBuffer &buffer = **buffer_or_error;
-    cpu_info = std::vector<uint8_t>(
-        reinterpret_cast<const uint8_t *>(buffer.getBufferStart()),
-        reinterpret_cast<const uint8_t *>(buffer.getBufferEnd()));
-  }
-  return *cpu_info;
+  static ErrorOr<std::unique_ptr<MemoryBuffer>> cpu_info_or_err =
+      getProcFile("cpuinfo");
+
+  if (!*cpu_info_or_err)
+    cpu_info_or_err.getError();
+
+  MemoryBuffer &buffer = **cpu_info_or_err;
+  return arrayRefFromStringRef(buffer.getBuffer());
 }
 
 Expected<std::vector<cpu_id_t>>
 lldb_private::process_linux::GetAvailableLogicalCoreIDs(StringRef cpuinfo) {
   SmallVector<StringRef, 8> lines;
   cpuinfo.split(lines, "\n", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
   std::vector<cpu_id_t> logical_cores;
 
   for (StringRef line : lines) {
     std::pair<StringRef, StringRef> key_value = line.split(':');