[ctx_profile] Pull ContextNode in a .h file
#91669
Conversation
This pulls out `ContextNode` as we need to use it pretty much as-is to implement a writer. The writer will be implemented on the LLVM side because it takes a dependency on BitStreamWriter. Since we can't reuse a header between compiler-rt and llvm, we use a .inc which is copied on both sides, and test that the 2 copies are identical. The changes adds the necessary other stuff for compiler-rt/ctx_profile testing.
|
✅ With the latest revision this PR passed the Python code formatter. |
| // NOTE! | ||
| // llvm/lib/ProfileData/CtxInstrContextNode.inc and | ||
| // compiler-rt/lib/ctx_profile/CtxInstrContextNode.inc | ||
| // must be exact copies of eachother |
There was a problem hiding this comment.
nit: space between each and other?
| /// to be concerned with memory safety. Their subcontexts never get populated, | ||
| /// though. The runtime code here produces and recognizes them. | ||
|
|
||
| using GUID = uint64_t; |
There was a problem hiding this comment.
I think we need a couple of things here:
- An include guard to make sure things don't get pulled in twice.
- Depending on where these things are pulled in from, they may end up with different namespaces. Usually we define the namespaces we want these to be in at this location.
There was a problem hiding this comment.
Well, that really says "this should be a header", which this isn't at the moment. There's some - but not lots - of advantage to that, but there is one challenge: what's the include guard going to be - because it won't be matching the guideline on the llvm side.
There was a problem hiding this comment.
Nevermind - made it a header. What was giving me pause is 2 things: one, what exactly should the include guard be, because there's a guideline about naming it that captures its path. Looks like compiler-rt doesn't really have that, so using LLVM as SoT. Second, standard includes. But the 2 I need are also on the compiler-rt, and since the goal of this type is to stay fairly low-level, it's probably fine.
There was a problem hiding this comment.
This is nice, is it possible to have this config shared across other compiler-rt projects?
There was a problem hiding this comment.
wdym - the other test dirs have a thing like this
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
mtrofin
changed the title
ContextNode in a .inc fileContextNode in a .h file
This pulls out
ContextNodeas we need to use it pretty much as-is to implement a writer. The writer will be implemented on the LLVM side because it takes a dependency on BitStreamWriter.Since we can't reuse a header between compiler-rt and llvm, we use a .inc which is copied on both sides, and test that the 2 copies are identical.
The changes adds the necessary other stuff for compiler-rt/ctx_profile testing.