Use common function for TypeError on illegal offset access#10544
Conversation
7f87332 to
f0d0e33
Compare
|
Would like to get opinions on the approach of using the Also, I've just used Opinions @arnaud-lb, @iluuu1994, @derickr ? |
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
I'm always in favor of improving error messages. It seems like this will be somewhat inconsistent though as some of these functions aren't aware of the BP and can be used for any of them, but propagating it just for the error message doesn't seem worth it.
f0d0e33 to
93e78d4
Compare
There was a problem hiding this comment.
I think this case shows we probably should improve the message somewhat. As this is inconsistent with trying to unset the offset of a string (which is banned in general). Maybe strings do need to be seperate :/
93e78d4 to
1d35b17
Compare
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
I didn't go through all the changes this time because it doesn't seem the BPs have changed.
There was a problem hiding this comment.
To be fully correct this would need to get the class name passed from the caller.
There was a problem hiding this comment.
I know, but that requires some more intrusive changes, which I want to do as a follow-up. As I first notices this being an issue with the ArrayObject case. But I think in both cases some places have hard coded the value of the class name.
There was a problem hiding this comment.
You replace the calls to specialized functions with the calls to a single universal one. This requires sending more arguments and therefore will increases the VM code size.
arnaud-lb
left a comment
arnaud-lb
left a comment
There was a problem hiding this comment.
Looks good to me appart from other comments
There was a problem hiding this comment.
Only a question, not a suggestion: What do you think of also specifying what is allowed? Array offset must be int or string, cannot access offset of type Closure
There was a problem hiding this comment.
We can only really do this for arrays and strings :/ as objects that implement ArrayOffset can in theory support any type
dstogov
left a comment
dstogov
left a comment
There was a problem hiding this comment.
I suggest to split ``zend_illegal_container_offset() back to cheaper specialized functions at least for the VM handlers.
This merges all usages of emiting an offset TypeError into a new ZEND_API function zend_illegal_container_offset(const char* container, const zval *offset, int type); Where the container should represent the type on which the access is attempted (e.g. string, array) The offset zval that is used, where the error message will display its type The type of access, which should be a BP_VAR_* constant, to get special message for isset/empty/unset
Also pass called CE
1d35b17 to
381e865
Compare
I hope what I've done is what you asked, should I do the same for the JIT VM handlers? |
381e865 to
c41efe0
Compare
dstogov
left a comment
dstogov
left a comment
There was a problem hiding this comment.
In this state, the patch shouldn't make any harm, but I also don't see any benefits.
I'm indifferent.
4 participants
Follow-up on #10504
This also fixes some consistency issues
Currently, using CI to check the JIT changes are correct as I am getting weird errors (possibly due to ASAN/UBSAN)