Mistake in BuiltinOperatorOverloadBuilder::getUsualArithmeticConversions in SemaOverload.cpp

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Mistake in BuiltinOperatorOverloadBuilder::getUsualArithmeticConversions in SemaOverload.cpp

Tom Stellard via cfe-dev
Hello,

Looks like the static table for arithmetic conversions inside
"getUsualArithmeticConversions" is out of date.

It is declared as 12x12 table but is initialized as 11x11 table. It
seems that the Float128 type is not handled in it, but it should be.

For example, in method "addGenericBinaryArithmeticOverloads" we iterate
over all promoted arithmetic types and Float128 is among them. It leads
to access to not initialized elements of the table and also, since the
Float128 type is 4th in method "getArithmeticType", all returned
conversions for types with index more than or equal to 4 may be wrong.

Thanks,
Petr Kudriavtsev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Mistake in BuiltinOperatorOverloadBuilder::getUsualArithmeticConversions in SemaOverload.cpp

Tom Stellard via cfe-dev
Happy Thursday,

As it turns out, we never seem to use the result of that function; we
just shove it into a struct field and ignore it. :) r304996 was landed
to clean this up.

Thank you!
George

On Wed, Jun 7, 2017 at 9:50 AM, Petr Kudryavtsev via cfe-dev
<[hidden email]> wrote:

> Hello,
>
> Looks like the static table for arithmetic conversions inside
> "getUsualArithmeticConversions" is out of date.
>
> It is declared as 12x12 table but is initialized as 11x11 table. It seems
> that the Float128 type is not handled in it, but it should be.
>
> For example, in method "addGenericBinaryArithmeticOverloads" we iterate over
> all promoted arithmetic types and Float128 is among them. It leads to access
> to not initialized elements of the table and also, since the Float128 type
> is 4th in method "getArithmeticType", all returned conversions for types
> with index more than or equal to 4 may be wrong.
>
> Thanks,
> Petr Kudriavtsev
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Mistake in BuiltinOperatorOverloadBuilder::getUsualArithmeticConversions in SemaOverload.cpp

Tom Stellard via cfe-dev
You are welcome:)

Maybe you also interested in the following:
In file PartialDiagnostic.h, field "PartialDiagnostic::Allocator"
sometimes is initialized with ~((uintptr_t)0) (looks like as a marker
that value in "DiagStorage" is owned by someone outside). But later, it
seems that in several methods that value handled improperly:
1) In method "getStorage" assertion should be moved inside "if
(Allocator)" branch, not "else" branch.
2) In "freeStorageSlow" branch "else if (Allocator !=
reinterpret_cast<StorageAllocator *>(~uintptr_t(0)))" is never executed
when "Allocator" is actually has that special value, so it probably
should be rearranged to be first and changed a bit:
Now it is:
     ...
     if (Allocator)
       Allocator->Deallocate(DiagStorage);
     else if (Allocator != reinterpret_cast<StorageAllocator
*>(~uintptr_t(0)))
       delete DiagStorage;
     ...
But should be:
     ...
     if (Allocator == reinterpret_cast<StorageAllocator *>(~uintptr_t(0))) {
       // do nothing
     } else if (Allocator) {
       Allocator->Deallocate(DiagStorage);
     } else {
       delete DiagStorage;
     }

Apparently, code was written under assumption that
"reinterpret_cast<StorageAllocator *>(~uintptr_t(0))" evaluates to 0 (or
false) but it doesn't seem right.

Thanks,
Petr

On 08.06.2017 21:27, George Burgess IV wrote:

> Happy Thursday,
>
> As it turns out, we never seem to use the result of that function; we
> just shove it into a struct field and ignore it. :) r304996 was landed
> to clean this up.
>
> Thank you!
> George
>
> On Wed, Jun 7, 2017 at 9:50 AM, Petr Kudryavtsev via cfe-dev
> <[hidden email]> wrote:
>> Hello,
>>
>> Looks like the static table for arithmetic conversions inside
>> "getUsualArithmeticConversions" is out of date.
>>
>> It is declared as 12x12 table but is initialized as 11x11 table. It seems
>> that the Float128 type is not handled in it, but it should be.
>>
>> For example, in method "addGenericBinaryArithmeticOverloads" we iterate over
>> all promoted arithmetic types and Float128 is among them. It leads to access
>> to not initialized elements of the table and also, since the Float128 type
>> is 4th in method "getArithmeticType", all returned conversions for types
>> with index more than or equal to 4 may be wrong.
>>
>> Thanks,
>> Petr Kudriavtsev
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Loading...