ChaiScript
Class with type_conversion from int and "==" operator causes switch statement to compare destroyed objects
- Compiler Used: Visual Studio 2015 Update 3
- Operating System: Windows 10
- Architecture (ARM/x86/32bit/64bit/etc): x64
Expected Behavior
A class added to ChaiScript that adds a type conversion from int as well as an "==" operator which compares objects by const reference unfortunately causes all comparison calls like 8==8 to go through that object rather than the normal Boxed_Number comparison. I guess this is expected, as the ChaiScript engine intentionally moves Boxed_Number functions lower in the ordering when deciding what function to call. Therefore, I'd expect a switch statement like this to create 2 objects of my class and compare them by invoking the object's operator==:
switch( 7 ) { case(7){} }
Actual Behavior
Two objects of that class do get created to compare, but get destroyed before the == function gets called (so we end up comparing 2 destroyed objects).
Minimal Example to Reproduce Behavior
struct S { S() = default; S( int N ) : n( N ) {} ~S() { n = INT_MIN; } bool operator==( const S& o ) { printf( "lhs = %d\nrhs = %d\n", n, o.n ); return n == o.n; } int n = 0; };
using namespace chaiscript; ChaiScript c;
c.add( user_type< S >(), "S" ); c.add( constructor< S(int) >(), "S" ); c.add( type_conversion< int, S >() ); c.add( fun( &S::operator== ), "==" );
c.eval( R"(
switch ( 7 ) { case( 7 ) {} } )" );
Incidentally, I wish I knew this before I added it to our product. I don't want to have every 8==8 function call run through my object, but I don't know how to extricate myself from this now that customers can have scripts that use my code. For all you developers out there using ChaiScript, DO NOT add a type_conversion from POD types or strings for your class, and if you do, DO NOT expose functions that overload functions that ChaiScript exposes, like ==, !=, <, >, <=, >=, or any other function. Otherwise, you will divert the ChaiScript engine from using expected behavior for comparing pod types through internal ChaiScript code; instead, your object will be created from the pod type and then your functions will be invoked.
There is a bigger issue at play here, and your patch should be unnecessary. There is specific code in ChaiScript that 1) checks if both objects are Arithmetic types and never calls an operator== if they are and 2) calling a function that requires conversions is the last possible thing that should happen.
So we should never be getting the conversion to your type in the first place.
Also, your notes imply that this is an issue for every integer operation, which should be quite impossible, but your patch only covers switch statements. We have to determine what the actual source of the issue is to make sure we are fixing the right thing.
Sorry, my description of the problem sort of combines two issues. First: if I have an object that exposes an == operator and a conversion from int, every time 2 ints are compared using == in the script, 2 of my objects get created and then compared. This is because ChaiScript does not register an == operator for 2 ints, but rather depends on the Boxed_Number == operator, and this means that a conversion WILL occur. Also, Boxed_Number functions are moved to the end of the functions that could be executed, so my object's == operator becomes the best choice. Of course, if I expose the other comparison operators like !=, <=, >=, >, < for my object, those will prevent any int to int comparison. None of this has anything to do with the comparison of destroyed objects.
The second bug is due to the above behavior. The above behavior may or may not be a bug, that depends on what you expect ChaiScript to do. The bug I fixed with my pull request has to do with the fact that Switch/Case statements use the == operator, but do it differently than how operator nodes call the == operator. My fix makes the 2 look alike, where we grab a reference to the 2 things that are being passed to the operator. Because of the behavior described above, 2 objects of my class get created for each case statement comparison. However, without my fix, the objects actually get destroyed before the operator == code is run. This can of course lead to very bad things.
To clarify:
- Outside of switch/case statements, the operator== and other comparisons using integers DO create my objects and use my object's operator==, but DO NOT compare destroyed objects. I wish it didn't, but it does. Perhaps I can fix that by adding operator== for each POD type, I will do some testing.
- Inside switch/case statements, the operator== DO create my objects and use my object's operator==, but DO destroy the objects BEFORE the operator== is executed. This leads to 2 uninitialized parts of memory to be compared, with unknown behavior as the result. This is the bug that the pull request helps with.
Don Inghram
On Sat, Mar 10, 2018 at 3:42 PM, Jason Turner [email protected] wrote:
There is a bigger issue at play here, and your patch should be unnecessary. There is specific code in ChaiScript that 1) checks if both objects are Arithmetic types and never calls an operator== if they are and 2) calling a function that requires conversions is the last possible thing that should happen.
So we should never be getting the conversion to your type in the first place.
Also, your notes imply that this is an issue for every integer operation, which should be quite impossible, but your patch only covers switch statements. We have to determine what the actual source of the issue is to make sure we are fixing the right thing.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ChaiScript/ChaiScript/issues/421#issuecomment-372073045, or mute the thread https://github.com/notifications/unsubscribe-auth/AWCfOeUgUMmS_e3ByXXTzz2rMiRT1-hMks5tdFa9gaJpZM4ScmQ4 .
Thank you for the clarification.
Just as an fyi, this is the source of my confusion
https://github.com/ChaiScript/ChaiScript/blob/develop/include/chaiscript/language/chaiscript_eval.hpp#L238
It really should never be executing any kind of function at all if two arithmetic operands are compared. So this speaks of a bigger issue.
Besides this I'll have to consider if a pure boxed number set of conversions should take priority over other conferences.
Jason
On Mar 11, 2018 16:14, "dinghram" [email protected] wrote:
Sorry, my description of the problem sort of combines two issues. First: if I have an object that exposes an == operator and a conversion from int, every time 2 ints are compared using == in the script, 2 of my objects get created and then compared. This is because ChaiScript does not register an == operator for 2 ints, but rather depends on the Boxed_Number == operator, and this means that a conversion WILL occur. Also, Boxed_Number functions are moved to the end of the functions that could be executed, so my object's == operator becomes the best choice. Of course, if I expose the other comparison operators like !=, <=, >=, >, < for my object, those will prevent any int to int comparison. None of this has anything to do with the comparison of destroyed objects.
The second bug is due to the above behavior. The above behavior may or may not be a bug, that depends on what you expect ChaiScript to do. The bug I fixed with my pull request has to do with the fact that Switch/Case statements use the == operator, but do it differently than how operator nodes call the == operator. My fix makes the 2 look alike, where we grab a reference to the 2 things that are being passed to the operator. Because of the behavior described above, 2 objects of my class get created for each case statement comparison. However, without my fix, the objects actually get destroyed before the operator == code is run. This can of course lead to very bad things.
To clarify:
- Outside of switch/case statements, the operator== and other comparisons using integers DO create my objects and use my object's operator==, but DO NOT compare destroyed objects. I wish it didn't, but it does. Perhaps I can fix that by adding operator== for each POD type, I will do some testing.
- Inside switch/case statements, the operator== DO create my objects and use my object's operator==, but DO destroy the objects BEFORE the operator== is executed. This leads to 2 uninitialized parts of memory to be compared, with unknown behavior as the result. This is the bug that the pull request helps with.
Don Inghram
On Sat, Mar 10, 2018 at 3:42 PM, Jason Turner [email protected] wrote:
There is a bigger issue at play here, and your patch should be unnecessary. There is specific code in ChaiScript that 1) checks if both objects are Arithmetic types and never calls an operator== if they are and 2) calling a function that requires conversions is the last possible thing that should happen.
So we should never be getting the conversion to your type in the first place.
Also, your notes imply that this is an issue for every integer operation, which should be quite impossible, but your patch only covers switch statements. We have to determine what the actual source of the issue is to make sure we are fixing the right thing.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/ 421#issuecomment-372073045>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWCfOeUgUMmS_ e3ByXXTzz2rMiRT1-hMks5tdFa9gaJpZM4ScmQ4> .
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ChaiScript/ChaiScript/issues/421#issuecomment-372154404, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOTJ1e_hCkuL7mKM4UQ0opS34gf7XjSks5tdaGzgaJpZM4ScmQ4 .
OK, you are right. It is amazing how you can look at something for a while and think you are seeing one thing, but just be flat out wrong. So, the do_oper is doing the right thing and that explains, in part, why I am not reproducing this bug outside of Switch. I thought the reason I wasn't seeing it outside Switch was because do_oper has Function_Push_Pop. I wonder now if the Switch statement should create a fake Binary_Operator_AST_Node object to use the do_oper. Or, optionally, we could make the do_oper function static and invoke it from Switch. Or, we could just duplicate the do_oper code to the Switch node processing. If you give me your preference, I can work on a new pull request with that version of the fix.
Don Inghram
On Sun, Mar 11, 2018 at 9:50 PM, Jason Turner [email protected] wrote:
Thank you for the clarification.
Just as an fyi, this is the source of my confusion
https://github.com/ChaiScript/ChaiScript/blob/develop/ include/chaiscript/language/chaiscript_eval.hpp#L238
It really should never be executing any kind of function at all if two arithmetic operands are compared. So this speaks of a bigger issue.
Besides this I'll have to consider if a pure boxed number set of conversions should take priority over other conferences.
Jason
On Mar 11, 2018 16:14, "dinghram" [email protected] wrote:
Sorry, my description of the problem sort of combines two issues. First: if I have an object that exposes an == operator and a conversion from int, every time 2 ints are compared using == in the script, 2 of my objects get created and then compared. This is because ChaiScript does not register an == operator for 2 ints, but rather depends on the Boxed_Number == operator, and this means that a conversion WILL occur. Also, Boxed_Number functions are moved to the end of the functions that could be executed, so my object's == operator becomes the best choice. Of course, if I expose the other comparison operators like !=, <=, >=, >, < for my object, those will prevent any int to int comparison. None of this has anything to do with the comparison of destroyed objects.
The second bug is due to the above behavior. The above behavior may or may not be a bug, that depends on what you expect ChaiScript to do. The bug I fixed with my pull request has to do with the fact that Switch/Case statements use the == operator, but do it differently than how operator nodes call the == operator. My fix makes the 2 look alike, where we grab a reference to the 2 things that are being passed to the operator. Because of the behavior described above, 2 objects of my class get created for each case statement comparison. However, without my fix, the objects actually get destroyed before the operator == code is run. This can of course lead to very bad things.
To clarify:
- Outside of switch/case statements, the operator== and other comparisons using integers DO create my objects and use my object's operator==, but DO NOT compare destroyed objects. I wish it didn't, but it does. Perhaps I can fix that by adding operator== for each POD type, I will do some testing.
- Inside switch/case statements, the operator== DO create my objects and use my object's operator==, but DO destroy the objects BEFORE the operator== is executed. This leads to 2 uninitialized parts of memory to be compared, with unknown behavior as the result. This is the bug that the pull request helps with.
Don Inghram
On Sat, Mar 10, 2018 at 3:42 PM, Jason Turner [email protected] wrote:
There is a bigger issue at play here, and your patch should be unnecessary. There is specific code in ChaiScript that 1) checks if both objects are Arithmetic types and never calls an operator== if they are and 2) calling a function that requires conversions is the last possible thing that should happen.
So we should never be getting the conversion to your type in the first place.
Also, your notes imply that this is an issue for every integer operation, which should be quite impossible, but your patch only covers switch statements. We have to determine what the actual source of the issue is to make sure we are fixing the right thing.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/ 421#issuecomment-372073045>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWCfOeUgUMmS_ e3ByXXTzz2rMiRT1-hMks5tdFa9gaJpZM4ScmQ4> .
— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/ 421#issuecomment-372154404>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOTJ1e_ hCkuL7mKM4UQ0opS34gf7XjSks5tdaGzgaJpZM4ScmQ4>
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ChaiScript/ChaiScript/issues/421#issuecomment-372186953, or mute the thread https://github.com/notifications/unsubscribe-auth/AWCfObb6gLRkgpp-piyxMza3cb2T1jpHks5tdfCagaJpZM4ScmQ4 .
Also, to fix the issue so that it doesn't have incorrect information in it, should I edit it and rewrite it, or update it with new notes that negate some of the statements I made?
Don
On Mon, Mar 12, 2018 at 9:07 AM, Don Inghram [email protected] wrote:
OK, you are right. It is amazing how you can look at something for a while and think you are seeing one thing, but just be flat out wrong. So, the do_oper is doing the right thing and that explains, in part, why I am not reproducing this bug outside of Switch. I thought the reason I wasn't seeing it outside Switch was because do_oper has Function_Push_Pop. I wonder now if the Switch statement should create a fake Binary_Operator_AST_Node object to use the do_oper. Or, optionally, we could make the do_oper function static and invoke it from Switch. Or, we could just duplicate the do_oper code to the Switch node processing. If you give me your preference, I can work on a new pull request with that version of the fix.
Don Inghram
On Sun, Mar 11, 2018 at 9:50 PM, Jason Turner [email protected] wrote:
Thank you for the clarification.
Just as an fyi, this is the source of my confusion
https://github.com/ChaiScript/ChaiScript/blob/develop/includ e/chaiscript/language/chaiscript_eval.hpp#L238
It really should never be executing any kind of function at all if two arithmetic operands are compared. So this speaks of a bigger issue.
Besides this I'll have to consider if a pure boxed number set of conversions should take priority over other conferences.
Jason
On Mar 11, 2018 16:14, "dinghram" [email protected] wrote:
Sorry, my description of the problem sort of combines two issues. First: if I have an object that exposes an == operator and a conversion from int, every time 2 ints are compared using == in the script, 2 of my objects get created and then compared. This is because ChaiScript does not register an == operator for 2 ints, but rather depends on the Boxed_Number == operator, and this means that a conversion WILL occur. Also, Boxed_Number functions are moved to the end of the functions that could be executed, so my object's == operator becomes the best choice. Of course, if I expose the other comparison operators like !=, <=, >=, >, < for my object, those will prevent any int to int comparison. None of this has anything to do with the comparison of destroyed objects.
The second bug is due to the above behavior. The above behavior may or may not be a bug, that depends on what you expect ChaiScript to do. The bug I fixed with my pull request has to do with the fact that Switch/Case statements use the == operator, but do it differently than how operator nodes call the == operator. My fix makes the 2 look alike, where we grab a reference to the 2 things that are being passed to the operator. Because of the behavior described above, 2 objects of my class get created for each case statement comparison. However, without my fix, the objects actually get destroyed before the operator == code is run. This can of course lead to very bad things.
To clarify:
- Outside of switch/case statements, the operator== and other comparisons using integers DO create my objects and use my object's operator==, but DO NOT compare destroyed objects. I wish it didn't, but it does. Perhaps I can fix that by adding operator== for each POD type, I will do some testing.
- Inside switch/case statements, the operator== DO create my objects and use my object's operator==, but DO destroy the objects BEFORE the operator== is executed. This leads to 2 uninitialized parts of memory to be compared, with unknown behavior as the result. This is the bug that the pull request helps with.
Don Inghram
On Sat, Mar 10, 2018 at 3:42 PM, Jason Turner <[email protected]
wrote:
There is a bigger issue at play here, and your patch should be unnecessary. There is specific code in ChaiScript that 1) checks if both objects are Arithmetic types and never calls an operator== if they are and 2) calling a function that requires conversions is the last possible thing that should happen.
So we should never be getting the conversion to your type in the first place.
Also, your notes imply that this is an issue for every integer operation, which should be quite impossible, but your patch only covers switch statements. We have to determine what the actual source of the issue is to make sure we are fixing the right thing.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/ 421#issuecomment-372073045>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWCfOeUgUMmS_ e3ByXXTzz2rMiRT1-hMks5tdFa9gaJpZM4ScmQ4> .
— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/421# issuecomment-372154404>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOTJ1e_h CkuL7mKM4UQ0opS34gf7XjSks5tdaGzgaJpZM4ScmQ4>
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ChaiScript/ChaiScript/issues/421#issuecomment-372186953, or mute the thread https://github.com/notifications/unsubscribe-auth/AWCfObb6gLRkgpp-piyxMza3cb2T1jpHks5tdfCagaJpZM4ScmQ4 .
FYI, making Binary_Operator_AST_Node<T>::do_oper a static/public and using it inside Switch_AST_Node does fix the bug with comparing destroyed objects. Is this the fix you'd like to see in the pull request?
Don
On Mon, Mar 12, 2018 at 9:09 AM, Don Inghram [email protected] wrote:
Also, to fix the issue so that it doesn't have incorrect information in it, should I edit it and rewrite it, or update it with new notes that negate some of the statements I made?
Don
On Mon, Mar 12, 2018 at 9:07 AM, Don Inghram [email protected] wrote:
OK, you are right. It is amazing how you can look at something for a while and think you are seeing one thing, but just be flat out wrong. So, the do_oper is doing the right thing and that explains, in part, why I am not reproducing this bug outside of Switch. I thought the reason I wasn't seeing it outside Switch was because do_oper has Function_Push_Pop. I wonder now if the Switch statement should create a fake Binary_Operator_AST_Node object to use the do_oper. Or, optionally, we could make the do_oper function static and invoke it from Switch. Or, we could just duplicate the do_oper code to the Switch node processing. If you give me your preference, I can work on a new pull request with that version of the fix.
Don Inghram
On Sun, Mar 11, 2018 at 9:50 PM, Jason Turner [email protected] wrote:
Thank you for the clarification.
Just as an fyi, this is the source of my confusion
https://github.com/ChaiScript/ChaiScript/blob/develop/includ e/chaiscript/language/chaiscript_eval.hpp#L238
It really should never be executing any kind of function at all if two arithmetic operands are compared. So this speaks of a bigger issue.
Besides this I'll have to consider if a pure boxed number set of conversions should take priority over other conferences.
Jason
On Mar 11, 2018 16:14, "dinghram" [email protected] wrote:
Sorry, my description of the problem sort of combines two issues. First: if I have an object that exposes an == operator and a conversion from int, every time 2 ints are compared using == in the script, 2 of my objects get created and then compared. This is because ChaiScript does not register an == operator for 2 ints, but rather depends on the Boxed_Number == operator, and this means that a conversion WILL occur. Also, Boxed_Number functions are moved to the end of the functions that could be executed, so my object's == operator becomes the best choice. Of course, if I expose the other comparison operators like !=, <=, >=, >, < for my object, those will prevent any int to int comparison. None of this has anything to do with the comparison of destroyed objects.
The second bug is due to the above behavior. The above behavior may or may not be a bug, that depends on what you expect ChaiScript to do. The bug I fixed with my pull request has to do with the fact that Switch/Case statements use the == operator, but do it differently than how operator nodes call the == operator. My fix makes the 2 look alike, where we grab a reference to the 2 things that are being passed to the operator. Because of the behavior described above, 2 objects of my class get created for each case statement comparison. However, without my fix, the objects actually get destroyed before the operator == code is run. This can of course lead to very bad things.
To clarify:
- Outside of switch/case statements, the operator== and other comparisons using integers DO create my objects and use my object's operator==, but DO NOT compare destroyed objects. I wish it didn't, but it does. Perhaps I can fix that by adding operator== for each POD type, I will do some testing.
- Inside switch/case statements, the operator== DO create my objects and use my object's operator==, but DO destroy the objects BEFORE the operator== is executed. This leads to 2 uninitialized parts of memory to be compared, with unknown behavior as the result. This is the bug that the pull request helps with.
Don Inghram
On Sat, Mar 10, 2018 at 3:42 PM, Jason Turner < [email protected]> wrote:
There is a bigger issue at play here, and your patch should be unnecessary. There is specific code in ChaiScript that 1) checks if both objects are Arithmetic types and never calls an operator== if they are and 2) calling a function that requires conversions is the last possible thing that should happen.
So we should never be getting the conversion to your type in the first place.
Also, your notes imply that this is an issue for every integer operation, which should be quite impossible, but your patch only covers switch statements. We have to determine what the actual source of the issue is to make sure we are fixing the right thing.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/ 421#issuecomment-372073045>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWCfOeUgUMmS_ e3ByXXTzz2rMiRT1-hMks5tdFa9gaJpZM4ScmQ4> .
— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/421#issueco mment-372154404>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOTJ1e_h CkuL7mKM4UQ0opS34gf7XjSks5tdaGzgaJpZM4ScmQ4>
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ChaiScript/ChaiScript/issues/421#issuecomment-372186953, or mute the thread https://github.com/notifications/unsubscribe-auth/AWCfObb6gLRkgpp-piyxMza3cb2T1jpHks5tdfCagaJpZM4ScmQ4 .
Alternatively, I could make the parser create a Binary_Operator_AST_Node as the first child of the Case_AST_Node. Then, the optimizer would have the opportunity to change it to a Fold_Right_Binary_Operator_AST_Node. Then, the switch would just eval_internal on the case's first node. To me, the only benefit here is to let the optimizer turn this into a Fold_Right_Binary_Operator_AST_Node, but I am not sure how valuable that would be.
Don
On Mon, Mar 12, 2018 at 9:57 AM, Don Inghram [email protected] wrote:
FYI, making Binary_Operator_AST_Node<T>::do_oper a static/public and using it inside Switch_AST_Node does fix the bug with comparing destroyed objects. Is this the fix you'd like to see in the pull request?
Don
On Mon, Mar 12, 2018 at 9:09 AM, Don Inghram [email protected] wrote:
Also, to fix the issue so that it doesn't have incorrect information in it, should I edit it and rewrite it, or update it with new notes that negate some of the statements I made?
Don
On Mon, Mar 12, 2018 at 9:07 AM, Don Inghram [email protected] wrote:
OK, you are right. It is amazing how you can look at something for a while and think you are seeing one thing, but just be flat out wrong. So, the do_oper is doing the right thing and that explains, in part, why I am not reproducing this bug outside of Switch. I thought the reason I wasn't seeing it outside Switch was because do_oper has Function_Push_Pop. I wonder now if the Switch statement should create a fake Binary_Operator_AST_Node object to use the do_oper. Or, optionally, we could make the do_oper function static and invoke it from Switch. Or, we could just duplicate the do_oper code to the Switch node processing. If you give me your preference, I can work on a new pull request with that version of the fix.
Don Inghram
On Sun, Mar 11, 2018 at 9:50 PM, Jason Turner [email protected] wrote:
Thank you for the clarification.
Just as an fyi, this is the source of my confusion
https://github.com/ChaiScript/ChaiScript/blob/develop/includ e/chaiscript/language/chaiscript_eval.hpp#L238
It really should never be executing any kind of function at all if two arithmetic operands are compared. So this speaks of a bigger issue.
Besides this I'll have to consider if a pure boxed number set of conversions should take priority over other conferences.
Jason
On Mar 11, 2018 16:14, "dinghram" [email protected] wrote:
Sorry, my description of the problem sort of combines two issues. First: if I have an object that exposes an == operator and a conversion from int, every time 2 ints are compared using == in the script, 2 of my objects get created and then compared. This is because ChaiScript does not register an == operator for 2 ints, but rather depends on the Boxed_Number == operator, and this means that a conversion WILL occur. Also, Boxed_Number functions are moved to the end of the functions that could be executed, so my object's == operator becomes the best choice. Of course, if I expose the other comparison operators like !=, <=, >=, >, < for my object, those will prevent any int to int comparison. None of this has anything to do with the comparison of destroyed objects.
The second bug is due to the above behavior. The above behavior may or may not be a bug, that depends on what you expect ChaiScript to do. The bug I fixed with my pull request has to do with the fact that Switch/Case statements use the == operator, but do it differently than how operator nodes call the == operator. My fix makes the 2 look alike, where we grab a reference to the 2 things that are being passed to the operator. Because of the behavior described above, 2 objects of my class get created for each case statement comparison. However, without my fix, the objects actually get destroyed before the operator == code is run. This can of course lead to very bad things.
To clarify:
- Outside of switch/case statements, the operator== and other comparisons using integers DO create my objects and use my object's operator==, but DO NOT compare destroyed objects. I wish it didn't, but it does. Perhaps I can fix that by adding operator== for each POD type, I will do some testing.
- Inside switch/case statements, the operator== DO create my objects and use my object's operator==, but DO destroy the objects BEFORE the operator== is executed. This leads to 2 uninitialized parts of memory to be compared, with unknown behavior as the result. This is the bug that the pull request helps with.
Don Inghram
On Sat, Mar 10, 2018 at 3:42 PM, Jason Turner < [email protected]> wrote:
There is a bigger issue at play here, and your patch should be unnecessary. There is specific code in ChaiScript that 1) checks if both objects are Arithmetic types and never calls an operator== if they are and 2) calling a function that requires conversions is the last possible thing that should happen.
So we should never be getting the conversion to your type in the first place.
Also, your notes imply that this is an issue for every integer operation, which should be quite impossible, but your patch only covers switch statements. We have to determine what the actual source of the issue is to make sure we are fixing the right thing.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/ 421#issuecomment-372073045>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWCfOeUgUMmS_ e3ByXXTzz2rMiRT1-hMks5tdFa9gaJpZM4ScmQ4> .
— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/421#issueco mment-372154404>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOTJ1e_h CkuL7mKM4UQ0opS34gf7XjSks5tdaGzgaJpZM4ScmQ4>
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ChaiScript/ChaiScript/issues/421#issuecomment-372186953, or mute the thread https://github.com/notifications/unsubscribe-auth/AWCfObb6gLRkgpp-piyxMza3cb2T1jpHks5tdfCagaJpZM4ScmQ4 .
Nevermind, I don't think we can create a Binary_Operator_AST_Node as the first child of the Case_AST_Node. This would require that we give it both the right and left hand sides of the operator== during construction, and that doesn't work as we'd need to pass in the value being given to the Switch statement, which is just too complicated in my opinion. I think the best approach is to re-use Binary_Operator_AST_Node::do_oper in the Switch processing code. I currently have it converted to a static/public so that Switch can call it, and I can deliver that as a pull request if you agree this is the best fix.
Don
On Mon, Mar 12, 2018 at 10:04 AM, Don Inghram [email protected] wrote:
Alternatively, I could make the parser create a Binary_Operator_AST_Node as the first child of the Case_AST_Node. Then, the optimizer would have the opportunity to change it to a Fold_Right_Binary_Operator_AST_Node. Then, the switch would just eval_internal on the case's first node. To me, the only benefit here is to let the optimizer turn this into a Fold_Right_Binary_Operator_AST_Node, but I am not sure how valuable that would be.
Don
On Mon, Mar 12, 2018 at 9:57 AM, Don Inghram [email protected] wrote:
FYI, making Binary_Operator_AST_Node<T>::do_oper a static/public and using it inside Switch_AST_Node does fix the bug with comparing destroyed objects. Is this the fix you'd like to see in the pull request?
Don
On Mon, Mar 12, 2018 at 9:09 AM, Don Inghram [email protected] wrote:
Also, to fix the issue so that it doesn't have incorrect information in it, should I edit it and rewrite it, or update it with new notes that negate some of the statements I made?
Don
On Mon, Mar 12, 2018 at 9:07 AM, Don Inghram [email protected] wrote:
OK, you are right. It is amazing how you can look at something for a while and think you are seeing one thing, but just be flat out wrong. So, the do_oper is doing the right thing and that explains, in part, why I am not reproducing this bug outside of Switch. I thought the reason I wasn't seeing it outside Switch was because do_oper has Function_Push_Pop. I wonder now if the Switch statement should create a fake Binary_Operator_AST_Node object to use the do_oper. Or, optionally, we could make the do_oper function static and invoke it from Switch. Or, we could just duplicate the do_oper code to the Switch node processing. If you give me your preference, I can work on a new pull request with that version of the fix.
Don Inghram
On Sun, Mar 11, 2018 at 9:50 PM, Jason Turner <[email protected]
wrote:
Thank you for the clarification.
Just as an fyi, this is the source of my confusion
https://github.com/ChaiScript/ChaiScript/blob/develop/includ e/chaiscript/language/chaiscript_eval.hpp#L238
It really should never be executing any kind of function at all if two arithmetic operands are compared. So this speaks of a bigger issue.
Besides this I'll have to consider if a pure boxed number set of conversions should take priority over other conferences.
Jason
On Mar 11, 2018 16:14, "dinghram" [email protected] wrote:
Sorry, my description of the problem sort of combines two issues. First: if I have an object that exposes an == operator and a conversion from int, every time 2 ints are compared using == in the script, 2 of my objects get created and then compared. This is because ChaiScript does not register an == operator for 2 ints, but rather depends on the Boxed_Number == operator, and this means that a conversion WILL occur. Also, Boxed_Number functions are moved to the end of the functions that could be executed, so my object's == operator becomes the best choice. Of course, if I expose the other comparison operators like !=, <=, >=, >, < for my object, those will prevent any int to int comparison. None of this has anything to do with the comparison of destroyed objects.
The second bug is due to the above behavior. The above behavior may or may not be a bug, that depends on what you expect ChaiScript to do. The bug I fixed with my pull request has to do with the fact that Switch/Case statements use the == operator, but do it differently than how operator nodes call the == operator. My fix makes the 2 look alike, where we grab a reference to the 2 things that are being passed to the operator. Because of the behavior described above, 2 objects of my class get created for each case statement comparison. However, without my fix, the objects actually get destroyed before the operator == code is run. This can of course lead to very bad things.
To clarify:
- Outside of switch/case statements, the operator== and other comparisons using integers DO create my objects and use my object's operator==, but DO NOT compare destroyed objects. I wish it didn't, but it does. Perhaps I can fix that by adding operator== for each POD type, I will do some testing.
- Inside switch/case statements, the operator== DO create my objects and use my object's operator==, but DO destroy the objects BEFORE the operator== is executed. This leads to 2 uninitialized parts of memory to be compared, with unknown behavior as the result. This is the bug that the pull request helps with.
Don Inghram
On Sat, Mar 10, 2018 at 3:42 PM, Jason Turner < [email protected]> wrote:
There is a bigger issue at play here, and your patch should be unnecessary. There is specific code in ChaiScript that 1) checks if both objects are Arithmetic types and never calls an operator== if they are and 2) calling a function that requires conversions is the last possible thing that should happen.
So we should never be getting the conversion to your type in the first place.
Also, your notes imply that this is an issue for every integer operation, which should be quite impossible, but your patch only covers switch statements. We have to determine what the actual source of the issue is to make sure we are fixing the right thing.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/ 421#issuecomment-372073045>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWCfOeUgUMmS_ e3ByXXTzz2rMiRT1-hMks5tdFa9gaJpZM4ScmQ4> .
— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/ChaiScript/ChaiScript/issues/421#issueco mment-372154404>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOTJ1e_h CkuL7mKM4UQ0opS34gf7XjSks5tdaGzgaJpZM4ScmQ4>
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ChaiScript/ChaiScript/issues/421#issuecomment-372186953, or mute the thread https://github.com/notifications/unsubscribe-auth/AWCfObb6gLRkgpp-piyxMza3cb2T1jpHks5tdfCagaJpZM4ScmQ4 .
Just to clarify, ChaiScript does compare objects after they are destroyed with switch statements as my original demo script above demonstrates. My latest pull request fixes the bug. Have you had a chance to review it?
Have you had a chance to look at this? Switch statements can compare objects after they are destroyed. You were right, this doesn't happen when operator== is called, but it does happen in switch statements.
Have you had a chance to look at this? Switch statements can compare objects after they are destroyed. You were right, this doesn't happen when operator== is called, but it does happen in switch statements. I updated my pull request to invoke the Binary_Operator_AST_Node<T>::do_oper, which does all the correct processing for arithmatic and non-arithmatic arguments. This actually simplifies the code, as we no longer need to try to "do the right thing" for comparison in two separate blocks of code. It also removes the bug, which is the most important thing.
Don
On Sat, Mar 10, 2018 at 3:42 PM, Jason Turner [email protected] wrote:
There is a bigger issue at play here, and your patch should be unnecessary. There is specific code in ChaiScript that 1) checks if both objects are Arithmetic types and never calls an operator== if they are and 2) calling a function that requires conversions is the last possible thing that should happen.
So we should never be getting the conversion to your type in the first place.
Also, your notes imply that this is an issue for every integer operation, which should be quite impossible, but your patch only covers switch statements. We have to determine what the actual source of the issue is to make sure we are fixing the right thing.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ChaiScript/ChaiScript/issues/421#issuecomment-372073045, or mute the thread https://github.com/notifications/unsubscribe-auth/AWCfOeUgUMmS_e3ByXXTzz2rMiRT1-hMks5tdFa9gaJpZM4ScmQ4 .
If you look at the diffs on the pull request, they look pretty crazy, because they are comparing to the previous pull request, which I undid. If you compare to trunk, the changes are simple and reasonable.