Improvements to RocprofLogger#1421
Conversation
- Check return codes of rocprofiler-sdk API calls and log errors - Avoid using .handle for opaque types directly - Improve finalization variable lifetime and ordering - use placement new into buffer in .bss section to prevent destruction of "GlobalContext" singleton - Implement RocprofLogger::endTracing() to force invocation of finalization function - Remove superfluous logging_ and registered_ variables - can be handled by queries to rocprofiler-sdk API
|
Hi @jrmadsen! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
I have signed the CLA. |
|
@jrmadsen, thank you! The bot isn't picking up that you signed the CLA. We're also getting a failure on the ROCm PyTorch tests, but it may be an infra issue, so I've kicked it off again. Can you also rebase from main? We've made some testing improvements in the past week that may also help with that. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@scotts has imported this pull request. If you are a Meta employee, you can view this in D108910661. |
| return instance; | ||
| // placement new into .bss section to prevent destruction issues on shutdown. | ||
| static auto *instance = new (global_context_buffer.data()) GlobalContext{}; | ||
| return *instance; |
There was a problem hiding this comment.
This is a legit, common trick (leaky singleton / immortal singleton).
Reserve a chunk of raw bytes big enough to hold a GlobalContext - the compiler sees plain bytes, not an object, so there's no destructor attached to it. Then construct a GlobalContext in this exact memory we reserved - instead of allocating fresh. Thus we get a real, live object, but since nothing owns it as a typed object with a destructor, it's never destroyed.
The problem:
std::array<std::byte, ...> only guarantees 1-byte alignment, because a single byte can sit anywhere. So the reserved buffer can start at any address, say one ending in ...3. Then placement-new drops a GlobalContext (and its std::mutex) onto that odd address. Now the mutex is misaligned: it's sitting somewhere the standard says it's never allowed to. That's undefined behavior. In practice it ranges from "works by luck" to torn reads, failed atomics, or crashes, and it's the kind of thing that's fine on one toolchain/arch and explodes on another.
Suggested patch:
alignas(GlobalContext) auto global_context_buffer =
std::array<std::byte, sizeof(GlobalContext)>{};
alignas(GlobalContext) forces the buffer's starting address to satisfy whatever GlobalContext's strictest member needs (the mutex, etc). Now placement-new lands the object on a legal address and the undefined behavior is gone.
There was a problem hiding this comment.
@sanrise, I think we should be even simpler:
GlobalContext& getGlobalContext() {
static GlobalContext* instance = new GlobalContext{}; // intentionally leaked
return *instance;
}
I'd rather code be obviously correct, and the alignment operator with placement new is not obvious. We also intentionally leak global singletons in other places. If intentionally leaking the global context becomes a problem, we can deal with it then - but I doubt it will.
| CHECKSTATUS), \ | ||
| rocprofiler_get_status_string(CHECKSTATUS)); \ | ||
| } \ | ||
| } |
There was a problem hiding this comment.
It's safer to wrap this in the conventional do { ... } while (0).
| } | ||
| } else if (status == 1) { | ||
| singleton().registered_ = true; | ||
| } |
There was a problem hiding this comment.
This is now an empty branch - we should delete it.
| auto& globalContext = getGlobalContext(); | ||
| globalContext.name_info = rocprofiler::sdk::get_callback_tracing_names(); | ||
| globalContext.buff_name_info = rocprofiler::sdk::get_buffer_tracing_names(); | ||
| globalContext.finalizer = finialize_func; |
There was a problem hiding this comment.
We have two error paths, but the finalizer is still set. I think that's invalid - we should probably only set the finalizer after we're certain init will succeed.
Also, might as well fix the typo: finialize_func -> finalize_func
| apiList.add("hipSetDevice"); | ||
| apiList.add("hipGetLastError"); | ||
| apiList.add("__hipPushCallConfiguration"); | ||
| apiList.add("__hipPopCallConfiguration"); |
There was a problem hiding this comment.
Is removing these purposeful?
|
@mwootton, I added some other minor comments. |
|
I pinged @jrmadsen. Removing the items from the filter list looks like a mistake, those are pure noise AFAIK. They wrap each kernel launch api. |
4 participants
GlobalContextsingletonRocprofLogger::endTracing()to force invocation of finalization function