Change reinterpret_cast use to memcpy #2295
Conversation
josalem
changed the title
| @@ -24,7 +24,7 @@ bool TryParse(uint8_t *&bufferCursor, uint32_t &bufferLen, T &result) | |||
|
|
|||
| if (bufferLen < sizeof(T)) | |||
| return false; | |||
| result = *(reinterpret_cast<T *>(bufferCursor)); | |||
| memcpy(&result, bufferCursor, sizeof(T)); | |||
There was a problem hiding this comment.
Might consider something like:
template <typename T>
static inline T unaligned_reinterpret_cast(const void* ptr)
{
T tmp;
memcpy(&tmp, ptr, sizeof(T));
return tmp;
}Then just use it like:
result = unaligned_reinterpret_cast<T>(bufferCursor);At least, for fundamental types such as int and float, this generates pretty good code that does not involve copies.
There was a problem hiding this comment.
I'm not sure I see how refactoring that code removes the need for a copy. memcpy happens either way. Are you suggesting replacing the entirety of TryParse with the above code?
There was a problem hiding this comment.
Oh, I mean, it still calls memcpy() (which might not even copy, just load it in a register), but there's no additional copy because it's returning tmp instead of a reference or pointer.
This just wraps memcpy() in a function in such a way that it's clear from the call site that it's a reinterpret_cast that works with unaligned pointers. It's otherwise identical to the direct memcpy() call in your patch.
| return false; | ||
| result = reinterpret_cast<const T *>(bufferCursor); | ||
|
|
||
| T *resultBuffer = new (nothrow) T[stringLen]; |
There was a problem hiding this comment.
Does this buffer ever get deleted? I didn't notice any changes at any callsite of TryParseString.
There was a problem hiding this comment.
I just updated the code so that strings are held in NewHolder|NewArrayHolders so they should get deleted correctly. I'm going to see about running Valgrind over it to make sure.
| @@ -24,7 +24,7 @@ bool TryParse(uint8_t *&bufferCursor, uint32_t &bufferLen, T &result) | |||
|
|
|||
| if (bufferLen < sizeof(T)) | |||
| return false; | |||
| result = *(reinterpret_cast<T *>(bufferCursor)); | |||
| memcpy(&result, bufferCursor, sizeof(T)); | |||
There was a problem hiding this comment.
Testing with this single line change, it just worked in my case.
| @@ -55,17 +55,19 @@ const GenerateCoreDumpCommandPayload* GenerateCoreDumpCommandPayload::TryParse(B | |||
| return nullptr; | |||
| } | |||
|
|
|||
| payload->incomingBuffer = lpBuffer; | |||
| uint8_t* pBufferCursor = payload->incomingBuffer; | |||
| uint8_t* pBufferCursor = lpBuffer; | |||
There was a problem hiding this comment.
Does GenerateCoreDumpCommandPayload* above need to be NewArrayHolder now?
There was a problem hiding this comment.
The user is expected to handle the memory new'd here. Down on line 88, the GenerateCoreDumpPayload is put into a NewHolder.
There was a problem hiding this comment.
It is a good practice to assign a newly allocated memory to a holder immediately. That way, you would not need to delete it at line 65 and it is also future proof when someone might add another return path to this function. The final return in this function would then use payload->Extract() or just call SuppressRelease() before the return.
|
After some discussion with @noahfalk last week, I'm going top scale back the scope of this PR to just the line that @swift-kim said solved their issue. This is minimally invasive and solves the issue. Once the PR is scaled back, I'll mark the PR as ready for review. |
|
@josalem Is there any news? |
@swift-kim I was offline last week, but I should be able to modify the PR this week to only include the line that fixes your issue and mark the PR for review. |
josalem
force-pushed
the
dev/josalem/rm-reinterpretcast
branch
from
293b498 to
b2ee74e
Compare
josalem
force-pushed
the
dev/josalem/rm-reinterpretcast
branch
from
b2ee74e to
0082ccd
Compare
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
Changes use of
reinterpret_casttomemcpyin EventPipe IPC read/write operations.resolves #2067
I'm going to be doing some local tests to convince myself that memory is still being accounted for. I'll flip the bit on PR status after that.
CC - @tommcdon