[turbofan, globals] Fix creating too long strings [v8/v8 : main]
Marja Hölttä (Gerrit)
Marja Hölttä added 1 comment![]( "Open in Gerrit")
Related details
- Darius Mercadier
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 added 3 comments![]( "Open in Gerrit")
(IsStringConstant(broker(), lhs) || IsStringConstant(broker(), rhs))) {Isn't that redundant? GetMaxStringLength above already checks this. This should probably be a DCHECK rather than part of the condition.
(and if it's not redundant, then it's plain wrong: should be `&&` instead of `||`)
// ConcatenateStrings won't fail, since we already checked the resulting
// string is not too long.So... ConcatenateStrings can fail then? How?
The commit message says `TF didn't sufficiently check against String::kMaxLength.` but line 379 does `*lhs_len + *rhs_len <= String::kMaxLength`, which looks like it should be enough, no?
if (compiler::utils::ConcatenateStrings(left, right, broker())Is that necessary? We're already in the compiler namesapce.
Related details
- Marja Hölttä
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted and added 4 comments![]( "Open in Gerrit")
Votes added by Marja Hölttä
| Commit-Queue | +1 |
4 comments
(IsStringConstant(broker(), lhs) || IsStringConstant(broker(), rhs))) {Isn't that redundant? GetMaxStringLength above already checks this. This should probably be a DCHECK rather than part of the condition.
(and if it's not redundant, then it's plain wrong: should be `&&` instead of `||`)
No, because GetMaxStringLength above also supports double constants. I think this branch is meant to execute when at least one is a string constant, but both don't need to be.
So this supports
"some string" + 123e4
and
1234e4 + "some string"
// ConcatenateStrings won't fail, since we already checked the resulting
// string is not too long.So... ConcatenateStrings can fail then? How?
The commit message says `TF didn't sufficiently check against String::kMaxLength.` but line 379 does `*lhs_len + *rhs_len <= String::kMaxLength`, which looks like it should be enough, no?
aaargh you're right, this was actually failing only because the max length of doubles was incorrectly set. Cancelling this part of the change.
if (compiler::utils::ConcatenateStrings(left, right, broker())Is that necessary? We're already in the compiler namesapce.
- Darius Mercadier
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
Related details
- Marja Hölttä
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted Commit-Queue+2![]( "Open in Gerrit")
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 added 1 comment![]( "Open in Gerrit")
(IsStringConstant(broker(), lhs) || IsStringConstant(broker(), rhs))) {Marja HölttäIsn't that redundant? GetMaxStringLength above already checks this. This should probably be a DCHECK rather than part of the condition.
(and if it's not redundant, then it's plain wrong: should be `&&` instead of `||`)
No, because GetMaxStringLength above also supports double constants. I think this branch is meant to execute when at least one is a string constant, but both don't need to be.
So this supports
"some string" + 123e4
and
1234e4 + "some string"
Mmmh ok, so it's preventing `123e4 + 123e4`.... why though?
Related details
- Marja Hölttä
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä added 1 comment![]( "Open in Gerrit")
(IsStringConstant(broker(), lhs) || IsStringConstant(broker(), rhs))) {Marja HölttäIsn't that redundant? GetMaxStringLength above already checks this. This should probably be a DCHECK rather than part of the condition.
(and if it's not redundant, then it's plain wrong: should be `&&` instead of `||`)
Darius MercadierNo, because GetMaxStringLength above also supports double constants. I think this branch is meant to execute when at least one is a string constant, but both don't need to be.
So this supports
"some string" + 123e4
and
1234e4 + "some string"
Mmmh ok, so it's preventing `123e4 + 123e4`.... why though?
Because that's not a string concatenation but number addition, so this code won't apply
Related details
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 added 1 comment![]( "Open in Gerrit")
(IsStringConstant(broker(), lhs) || IsStringConstant(broker(), rhs))) {Marja HölttäIsn't that redundant? GetMaxStringLength above already checks this. This should probably be a DCHECK rather than part of the condition.
(and if it's not redundant, then it's plain wrong: should be `&&` instead of `||`)
Darius MercadierNo, because GetMaxStringLength above also supports double constants. I think this branch is meant to execute when at least one is a string constant, but both don't need to be.
So this supports
"some string" + 123e4
and
1234e4 + "some string"
Marja HölttäMmmh ok, so it's preventing `123e4 + 123e4`.... why though?
Because that's not a string concatenation but number addition, so this code won't apply
Ah yea my bad, I assumed that we were in ReduceStringConcat or something, but you're right, we're just in ReduceJSAdd so still not sure about whether we'll create a string or not. Ok all good, thanks :)
Related details
- Marja Hölttä
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[globals] Fix creating too long strings
Our kMaxDoubleStringLength was wrong. E.g., the string representation of
-1.2345678901234567e-6 is -0.0000012345678901234567 which is 25
characters long.
- M src/common/globals.h
- A test/mjsunit/compiler/regress-498818400.js
Code-Review: +1 by Darius Mercadier
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |