Fix `FixedArray::length` on big endian [v8/v8 : main]
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Related details
- Arash Kazemi
- Darius Mercadier
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand voted and added 1 comment![]( "Open in Gerrit")
Votes added by Milad Farazmand
| Commit-Queue | +1 |
1 comment
This leaves `padding_` uninitialized on BE but since no test is failing I assume it's not being accessed, in which case why don't we make `MachineType::Uint32()` unconditional under `ForFixedArrayLength()`? why is `MachineType::Uint64()` even needed?
Related details
- Arash Kazemi
- Darius Mercadier
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Arash Kazemi
| Code-Review | +1 |
2 comments
This leaves `padding_` uninitialized on BE but since no test is failing I assume it's not being accessed, in which case why don't we make `MachineType::Uint32()` unconditional under `ForFixedArrayLength()`? why is `MachineType::Uint64()` even needed?
On little endian, I would prefer having all memory to be initialized, the workaround should be temporary as we phase out TF.
- Darius Mercadier
- Milad Farazmand
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Code-Review | +1 |
1 comment
- Milad Farazmand
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Arash KazemiThis leaves `padding_` uninitialized on BE but since no test is failing I assume it's not being accessed, in which case why don't we make `MachineType::Uint32()` unconditional under `ForFixedArrayLength()`? why is `MachineType::Uint64()` even needed?
On little endian, I would prefer having all memory to be initialized, the workaround should be temporary as we phase out TF.
Sounds good, thank you both for reviewing.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
Fix `FixedArray::length` on big endian
Needed after the changes made by http://crrev.com/c/7594595.
The fix is to use the same implementation as pointer compressed
builds.
- M src/compiler/access-builder.cc
- M src/compiler/allocation-builder-inl.h
- M src/compiler/js-create-lowering.cc
- M src/compiler/turboshaft/machine-lowering-reducer-inl.h
Code-Review: +1 by Arash Kazemi, +1 by Darius Mercadier
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Arash KazemiThis leaves `padding_` uninitialized on BE but since no test is failing I assume it's not being accessed, in which case why don't we make `MachineType::Uint32()` unconditional under `ForFixedArrayLength()`? why is `MachineType::Uint64()` even needed?
Milad FarazmandOn little endian, I would prefer having all memory to be initialized, the workaround should be temporary as we phase out TF.
Sounds good, thank you both for reviewing.
Hello, sorry to bug you again,
Just wanted to ask, you don't suspect this uninitialized padding_ will become an issue for us in the mean time correct?
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi added 1 comment![]( "Open in Gerrit")
Arash KazemiThis leaves `padding_` uninitialized on BE but since no test is failing I assume it's not being accessed, in which case why don't we make `MachineType::Uint32()` unconditional under `ForFixedArrayLength()`? why is `MachineType::Uint64()` even needed?
Milad FarazmandOn little endian, I would prefer having all memory to be initialized, the workaround should be temporary as we phase out TF.
Milad FarazmandSounds good, thank you both for reviewing.
Hello, sorry to bug you again,
Just wanted to ask, you don't suspect this uninitialized padding_ will become an issue for us in the mean time correct?
I doubt there will be future changes related to the uninitialized padding, I just think that having uninitialized memory even if not accessed could create difficult to debug issues. The current testing or fuzzing might not cover this case because the length used to be a Smi.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Arash KazemiThis leaves `padding_` uninitialized on BE but since no test is failing I assume it's not being accessed, in which case why don't we make `MachineType::Uint32()` unconditional under `ForFixedArrayLength()`? why is `MachineType::Uint64()` even needed?
Milad FarazmandOn little endian, I would prefer having all memory to be initialized, the workaround should be temporary as we phase out TF.
Milad FarazmandSounds good, thank you both for reviewing.
Arash KazemiHello, sorry to bug you again,
Just wanted to ask, you don't suspect this uninitialized padding_ will become an issue for us in the mean time correct?
I doubt there will be future changes related to the uninitialized padding, I just think that having uninitialized memory even if not accessed could create difficult to debug issues. The current testing or fuzzing might not cover this case because the length used to be a Smi.
Make sense, so do you think the current BE workaround will remain in place even after TF is removed? I just want to see if we need to invest time and add the initialization to BE as well or we can leave it as is.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arash Kazemi (Gerrit)
Arash Kazemi added 1 comment![]( "Open in Gerrit")
Arash KazemiThis leaves `padding_` uninitialized on BE but since no test is failing I assume it's not being accessed, in which case why don't we make `MachineType::Uint32()` unconditional under `ForFixedArrayLength()`? why is `MachineType::Uint64()` even needed?
Milad FarazmandOn little endian, I would prefer having all memory to be initialized, the workaround should be temporary as we phase out TF.
Milad FarazmandSounds good, thank you both for reviewing.
Arash KazemiHello, sorry to bug you again,
Just wanted to ask, you don't suspect this uninitialized padding_ will become an issue for us in the mean time correct?
Milad FarazmandI doubt there will be future changes related to the uninitialized padding, I just think that having uninitialized memory even if not accessed could create difficult to debug issues. The current testing or fuzzing might not cover this case because the length used to be a Smi.
Make sense, so do you think the current BE workaround will remain in place even after TF is removed? I just want to see if we need to invest time and add the initialization to BE as well or we can leave it as is.
The workaround is only for Turbofan, in maglev we already use 2 uint32_t values. So once we remove TF this code including the BE part will be removed.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Arash KazemiThis leaves `padding_` uninitialized on BE but since no test is failing I assume it's not being accessed, in which case why don't we make `MachineType::Uint32()` unconditional under `ForFixedArrayLength()`? why is `MachineType::Uint64()` even needed?
Milad FarazmandOn little endian, I would prefer having all memory to be initialized, the workaround should be temporary as we phase out TF.
Milad FarazmandSounds good, thank you both for reviewing.
Arash KazemiHello, sorry to bug you again,
Just wanted to ask, you don't suspect this uninitialized padding_ will become an issue for us in the mean time correct?
Milad FarazmandI doubt there will be future changes related to the uninitialized padding, I just think that having uninitialized memory even if not accessed could create difficult to debug issues. The current testing or fuzzing might not cover this case because the length used to be a Smi.
Arash KazemiMake sense, so do you think the current BE workaround will remain in place even after TF is removed? I just want to see if we need to invest time and add the initialization to BE as well or we can leave it as is.
The workaround is only for Turbofan, in maglev we already use 2 uint32_t values. So once we remove TF this code including the BE part will be removed.
Thank you so much for confirming.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |