Re: [Maria-developers] [Commits] Rev 2992: Removed some alias warnings in lp:maria/5.3
Michael Widenius <monty@askmonty.org> writes:
------------------------------------------------------------ revno: 2992 revision-id: monty@askmonty.org-20110517214756-6f7fv5uk68i4hb3b parent: sanja@askmonty.org-20110516132109-h2g9qzrfq3g9t6em committer: Michael Widenius <monty@askmonty.org> branch nick: maria-5.3 timestamp: Wed 2011-05-18 00:47:56 +0300 message: Removed some alias warnings Fixed alias bug when compiling with gcc 4.2.4 that caused subselect.test to fail
"Fix strict aliasing bug that caused subselect.test to fail" ("alias bug" can mean different things. And the problem is not GCC specific, see below)
=== modified file 'sql/item_subselect.cc' --- a/sql/item_subselect.cc 2011-05-16 12:07:04 +0000 +++ b/sql/item_subselect.cc 2011-05-17 21:47:56 +0000
@@ -3563,7 +3562,15 @@ subselect_single_select_engine::change_r } else result= res; - return select_lex->join->change_result(result); + + /* + We can't use 'result' below as gcc 4.2.4's alias optimization + assumes that result was not changed by thd->change_item_tree(). + I tried to find a solution to make gcc happy, but could not find anything + that would not require a lot of extra code that would be harder to manage + than the current code. + */ + return select_lex->join->change_result(res); }
I do not agree with this patch. Instead do something like this: @@ -3559,7 +3559,9 @@ subselect_single_select_engine::change_r nothing special about Item* pointer so it is safe conversion. We do not change the interface to be compatible with MySQL. */ - thd->change_item_tree((Item**) &result, (Item*)res); + Item *temp_item; + thd->change_item_tree(&temp_item, (Item*)res); + result= (select_result_interceptor *)temp_item; } else result= res; This makes the code correct (at least with respect to standard C++ strict aliasing), not just work-around the problem for one specific combination of compiler flags for one specific compiler version. The problem has nothing to do with any particular GCC version (except that this is where we happened to detect it), the problem is passing a select_result_interceptor** when an Item** is needed. This is in violation of the strict aliasing rule in standard C++. ---- However, I think we urgently need to start using -fno-strict-aliasing (and probably -fno-strict-overflow as well). Davi told me MySQL@Oracle is doing this as well. In effect there are two dialects of C/C++, even though they are rarely identified as such. - "System C/C++", which is a high-level assembler, where everything is bytes and pointers are just 8 (or 4) byte integers. - "Application C/C++", which is a type-safe language, and pointers are abstract objects that cannot be arbitrarily manipulated. Clearly, many (most?) developers are programming in "System C/C++", including you. That is fine, Postgress, the Linux Kernel, they use the same dialect. However, GCC by default uses "Application C/C++" (-fstrict-aliasing). We need to tell GCC which dialict of C/C++ we are using (-fno-strict-aliasing for "System C/C++"). If we could just agree on which dialect we use, and let GCC know our choice, things will be much better. - Kristian.
Hi!
"Kristian" == Kristian Nielsen <knielsen@knielsen-hq.org> writes:
<cut>
=== modified file 'sql/item_subselect.cc' --- a/sql/item_subselect.cc 2011-05-16 12:07:04 +0000 +++ b/sql/item_subselect.cc 2011-05-17 21:47:56 +0000
@@ -3563,7 +3562,15 @@ subselect_single_select_engine::change_r } else result= res; - return select_lex->join->change_result(result); + + /* + We can't use 'result' below as gcc 4.2.4's alias optimization + assumes that result was not changed by thd->change_item_tree(). + I tried to find a solution to make gcc happy, but could not find anything + that would not require a lot of extra code that would be harder to manage + than the current code. + */ + return select_lex->join->change_result(res); }
Kristian> I do not agree with this patch. The patch is correct in all aspect and it removes all alias problems for this function. The fact that 'result' is not detected to be changed is not a problem anymore. Just so you know, Timour and I spent 30 minutes going trough all possible ways (I come up with 6 of them) to fix the above problem, and the above was the cleanest one. Kristian> Instead do something like this: Kristian> @@ -3559,7 +3559,9 @@ subselect_single_select_engine::change_r Kristian> nothing special about Item* pointer so it is safe conversion. We do Kristian> not change the interface to be compatible with MySQL. Kristian> */ Kristian> - thd->change_item_tree((Item**) &result, (Item*)res); Kristian> + Item *temp_item; Kristian> + thd->change_item_tree(&temp_item, (Item*)res); Kristian> + result= (select_result_interceptor *)temp_item; Kristian> } Kristian> else Kristian> result= res; The above doesn't work. The reason is that we want to remember the position and value of &result so that we can later restore it. The above would get us to remember the value of &temp_item (somewhere on the stack) and later when we try to restore the original value we will overwrite some random position. <cut> Kristian> The problem has nothing to do with any particular GCC version (except that Kristian> this is where we happened to detect it), the problem is passing a Kristian> select_result_interceptor** when an Item** is needed. This is in violation of Kristian> the strict aliasing rule in standard C++. I agree and we tried to find a good solution for this. I tried even to change the function thd->change_item_tree() to work with generic pointers (void *), as it's not really item related but it didn't help :( Kristian> However, I think we urgently need to start using -fno-strict-aliasing (and Kristian> probably -fno-strict-overflow as well). Davi told me MySQL@Oracle is doing Kristian> this as well. Why, as we know that from testing that we in reality have very few alias problems and the -fno-strict-aliasing will have a notable speed impact. It would be better to go trough and try to fix all reported alias problems (yes, I know that there is a bunch of them but not that many that they can't be fixed). Don't know about the effects of -fno-strict-overflow is. What I don't like with how alias works, is that you can't take a pointer of a pointer of a derived class and cast it to it's base class. Item_derived *a; Item *b = (Item**) &a; You can't solve this by using a temporary item, as what you need to remember is a pointer to the original item. The only way I know of how to solve this is to create an union of all types and store things trough it. union Item_ptr_all { Item **item_ptr; Item_derived **item_derived_ptr;} And then do: Item_derived *a; Item_ptr_all c.item_derived_ptr= &a; Item *b= c.item_ptr; Which is far from nice. Kristian, do you know of any better way? Kristian> In effect there are two dialects of C/C++, even though they are rarely Kristian> identified as such. Kristian> - "System C/C++", which is a high-level assembler, where everything is bytes Kristian> and pointers are just 8 (or 4) byte integers. Kristian> - "Application C/C++", which is a type-safe language, and pointers are Kristian> abstract objects that cannot be arbitrarily manipulated. Kristian> Clearly, many (most?) developers are programming in "System C/C++", including Kristian> you. That is fine, Postgress, the Linux Kernel, they use the same dialect. Kristian> However, GCC by default uses "Application C/C++" (-fstrict-aliasing). We need Kristian> to tell GCC which dialict of C/C++ we are using (-fno-strict-aliasing for Kristian> "System C/C++"). Kristian> If we could just agree on which dialect we use, and let GCC know our choice, Kristian> things will be much better. The problem is not only if it's appliciton or system, it's also about getting the best possible code from the compiler. The major alias problem we have is that we want to store pointers to pointers of items. If we would create an union for this, I think we can get rid of most alias bugs. Knielsen, lets discuss this on IRC when you are back after your Friday holyday. I would *really* like to know if there is a better solution to the pointer to pointers than the union... Regards, Monty
participants (2)
-
Kristian Nielsen -
Michael Widenius