Fix Hash(c10::Scalar), account for garbage data in union #68201
Conversation
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit c77d02a (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
This pull request was exported from Phabricator. Differential Revision: D32367564 |
test/cpp/lazy/test_misc.cpp
Outdated
There was a problem hiding this comment.
Can we do static_cast<uint8_t*> here? Can you explain the memory layout here a bit such that it's easier for me to understand why writing to "offset 0" makes sense? Is there any way we can tell the differences from a to b such that we can also verify that this operation succeeds?
There was a problem hiding this comment.
There isn't any way to verify it without violating the API of the Scalar. I didn't know the memory layout either, except by experimenting. I found it's sizeof() was 32 bytes, which surprised me. I don't know exactly what memory i'm stepping on here, but, for the purposes of this experiment it's not important as long as it isn't memory used by the Long member field, which it isn't.
I did verify it though, by observing that the below EXPECTs all failed when I clobber this memory without fixing the hash function.
I don't see the point of changing the cast. I don't benefit from any compiler safety checks here, in fact, i'm doing something 'unsafe' on purpose.
There was a problem hiding this comment.
Just for you, I decided to try this :)
But it doesn't appear to work:
Static_cast from 'c10::Scalar *' to 'uint8_t *' (aka 'unsigned char *') is not allowed
Anyway, regular cast is fine.
There was a problem hiding this comment.
Yea, my bad. It should be reinterpret_cast for your use case:
*(reinterpret_cast<uint8_t*>(&b)) = 1;.
There was a problem hiding this comment.
Did a little research. It seems non-trivial to find any resources to answer me the memory layout question. So, it's fine to keep the comment intact.
Summary: Pull Request resolved: pytorch#68201 Hash(c10::Scalar) made a bad assumption that it was valid to just hash over all the bytes of data of the c10::Scalar struct. Becuase c10::Scalar stores a union of different (float/int/complex) types with different sizes, not all bytes are valid in all cases. Hash() should only read the bytes corresponding to the currently active type. Test Plan: Added new unit tests. Verified HashTest.Scalar failed with the original Hash() impl and then fixed. Differential Revision: D32367564 fbshipit-source-id: be1dbc4932890ebb70898184483b6776b068c6b0
|
This pull request was exported from Phabricator. Differential Revision: D32367564 |
wconstab
force-pushed
the
export-D32367564
branch
from
c36e18e to
d34527e
Compare
|
This pull request was exported from Phabricator. Differential Revision: D32367564 |
wconstab
force-pushed
the
export-D32367564
branch
from
d34527e to
c77d02a
Compare
|
This pull request has been merged in dc24503. |
Summary: Pull Request resolved: #68201 Hash(c10::Scalar) made a bad assumption that it was valid to just hash over all the bytes of data of the c10::Scalar struct. Becuase c10::Scalar stores a union of different (float/int/complex) types with different sizes, not all bytes are valid in all cases. Hash() should only read the bytes corresponding to the currently active type. Test Plan: Added new unit tests. Verified HashTest.Scalar failed with the original Hash() impl and then fixed. Reviewed By: alanwaketan Differential Revision: D32367564 fbshipit-source-id: ac30dd4f6dd0513954986d3d23c0c11ba802c37b
Summary: Pull Request resolved: #68201 Hash(c10::Scalar) made a bad assumption that it was valid to just hash over all the bytes of data of the c10::Scalar struct. Becuase c10::Scalar stores a union of different (float/int/complex) types with different sizes, not all bytes are valid in all cases. Hash() should only read the bytes corresponding to the currently active type. Test Plan: Added new unit tests. Verified HashTest.Scalar failed with the original Hash() impl and then fixed. Reviewed By: alanwaketan Differential Revision: D32367564 fbshipit-source-id: ac30dd4f6dd0513954986d3d23c0c11ba802c37b
3 participants
Summary:
Hash(c10::Scalar) made a bad assumption that it was valid to just hash over all the bytes of data of the c10::Scalar struct.
Becuase c10::Scalar stores a union of different (float/int/complex) types with different sizes, not all bytes are valid in all cases. Hash() should only read the bytes corresponding to the currently active type.
Test Plan: Added new unit tests. Verified HashTest.Scalar failed with the original Hash() impl and then fixed.
Differential Revision: D32367564