This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by wallace on Aug 3 2022, 10:01 AM.

Details

Summary

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

All relevant tests pass.

Diff Detail

Event Timeline

wallace created this revision.Aug 3 2022, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2022, 10:01 AM
wallace requested review of this revision.Aug 3 2022, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2022, 10:01 AM
labath added a subscriber: labath.Aug 3 2022, 10:12 AM
labath added inline comments.
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 updated this revision to Diff 449724.Aug 3 2022, 11:09 AM

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

lgtm. Pavel?

labath added a comment.Aug 4 2022, 5:52 AM

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 added inline comments.Aug 4 2022, 10:40 AM
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 updated this revision to Diff 450226.Aug 4 2022, 10:08 PM

simplify this diff following @labath's advice

labath accepted this revision.Aug 8 2022, 5:33 AM
labath added inline comments.
lldb/source/Plugins/Process/Linux/Procfs.cpp
27–29

return arrayRefFromStringRef(buffer.getBuffer());

This revision is now accepted and ready to land.Aug 8 2022, 5:33 AM

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.

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.

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