maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Kristian" == Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx> 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