[7/7] Libsanitizer merge from upstream r249633.

This is the final patch. Force libsanitizer to use an old ABI for ubsan 
float cast data descriptors, because for some exprs (e.g. that type of 
tcc_declaration) we can't get the right location for now. I'm not sure 
about this, perhaps it should be fixed in GCC somehow.

On Tue, Oct 13, 2015 at 02:22:36PM +0300, Maxim Ostapenko wrote:
> This is the final patch. Force libsanitizer to use an old ABI for ubsan
> float cast data descriptors, because for some exprs (e.g. that type of
> tcc_declaration) we can't get the right location for now. I'm not sure about
> this, perhaps it should be fixed in GCC somehow.

I don't like this (neither the heuristics on the libubsan, it wouldn't be a
big deal to add a new library entrypoint).
If because of the heuristics you need to ensure that the SourceLocation is
always known, then either you check in ubsan.c whether expand_location
gives you NULL xloc.file and in that case use old style float cast overflow
(without location) - i.e. pass 0, NULL, otherwise you use new style, i.e.
pass 1, &loc.  Or arrange through some special option to emit something like
{ "<unknown>", 0, 0 } instead of { NULL, 0, 0 } for the float cast case.
And, regardless of this, any progress in making sure we have fewer cases
with UNKNOWN_LOCATION on this will not hurt.  I think at this point I'd
prefer the first choice, i.e. using old style for locations without
filename, and new style otherwise.

> 2015-10-13  Maxim Ostapenko  <m.ostapenko@partner.samsung.com>
> 
> 	* ubsan/ubsan_handlers.cc (looksLikeFloatCastOverflowDataV1): Always
> 	return true for now.
> 
> Index: libsanitizer/ubsan/ubsan_handlers.cc
> ===================================================================
> --- libsanitizer/ubsan/ubsan_handlers.cc	(revision 250059)
> +++ libsanitizer/ubsan/ubsan_handlers.cc	(working copy)
> @@ -307,6 +307,9 @@
>  }
>  
>  static bool looksLikeFloatCastOverflowDataV1(void *Data) {
> +  // (TODO): propagate SourceLocation into DataDescriptor and use this
> +  // heuristic than.
> +  return true;
>    // First field is either a pointer to filename or a pointer to a
>    // TypeDescriptor.
>    u8 *FilenameOrTypeDescriptor;


	Jakub

On 14/10/15 10:48, Jakub Jelinek wrote:
> On Tue, Oct 13, 2015 at 02:22:36PM +0300, Maxim Ostapenko wrote:
>> This is the final patch. Force libsanitizer to use an old ABI for ubsan
>> float cast data descriptors, because for some exprs (e.g. that type of
>> tcc_declaration) we can't get the right location for now. I'm not sure about
>> this, perhaps it should be fixed in GCC somehow.
> I don't like this (neither the heuristics on the libubsan, it wouldn't be a
> big deal to add a new library entrypoint).
> If because of the heuristics you need to ensure that the SourceLocation is
> always known, then either you check in ubsan.c whether expand_location
> gives you NULL xloc.file and in that case use old style float cast overflow
> (without location) - i.e. pass 0, NULL, otherwise you use new style, i.e.
> pass 1, &loc.  Or arrange through some special option to emit something like
> { "<unknown>", 0, 0 } instead of { NULL, 0, 0 } for the float cast case.
> And, regardless of this, any progress in making sure we have fewer cases
> with UNKNOWN_LOCATION on this will not hurt.  I think at this point I'd
> prefer the first choice, i.e. using old style for locations without
> filename, and new style otherwise.
>
>> 2015-10-13  Maxim Ostapenko  <m.ostapenko@partner.samsung.com>
>>
>> 	* ubsan/ubsan_handlers.cc (looksLikeFloatCastOverflowDataV1): Always
>> 	return true for now.
>>
>> Index: libsanitizer/ubsan/ubsan_handlers.cc
>> ===================================================================
>> --- libsanitizer/ubsan/ubsan_handlers.cc	(revision 250059)
>> +++ libsanitizer/ubsan/ubsan_handlers.cc	(working copy)
>> @@ -307,6 +307,9 @@
>>   }
>>   
>>   static bool looksLikeFloatCastOverflowDataV1(void *Data) {
>> +  // (TODO): propagate SourceLocation into DataDescriptor and use this
>> +  // heuristic than.
>> +  return true;
>>     // First field is either a pointer to filename or a pointer to a
>>     // TypeDescriptor.
>>     u8 *FilenameOrTypeDescriptor;
>
> 	Jakub
>

Ok, got it. The first solution would require changes in libsanitizer 
because heuristic doesn't work for GCC, so perhaps new UBSan entry point 
should go upstream, right? Or this may be implemented as local patch for 
GCC?

BTW, I actually saw UNKNOWN_LOCATION for this expr:

volatile double var;  // this is tcc_decaration, so we have 
UNKNOWN_LOCATION for it.

I wonder if we need emit __ubsan_handle_float_cast_overflow here at all.

On Wed, Oct 14, 2015 at 01:51:44PM +0300, Maxim Ostapenko wrote:
> Ok, got it. The first solution would require changes in libsanitizer because
> heuristic doesn't work for GCC, so perhaps new UBSan entry point should go
> upstream, right? Or this may be implemented as local patch for GCC?

No.  The heuristics relies on:
1) either it is old style float cast overflow without location
2) or it is new style float cast with location, but the location must:
   a) not have NULL filename
   b) the filename must not be ""
   c) the filename must not be "\1"
So, my proposal was to emit in GCC the old style float cast overflow if a), b) or
c) is true, otherwise the new style.  I have no idea what you mean by
heuristic doesn't work for GCC after that.

> BTW, I actually saw UNKNOWN_LOCATION for this expr:
> 
> volatile double var;  // this is tcc_decaration, so we have UNKNOWN_LOCATION
> for it.

This is not a complete testcase, so I wonder what exactly you are talking
about.  The above doesn't not generate any
__ubsan_handle_float_cast_overflow calls with
-fsanitize=float-cast-overflow, and
volatile double d;
int bar (void) { return d; }
has location.

	Jakub

On 14/10/15 14:06, Jakub Jelinek wrote:
> On Wed, Oct 14, 2015 at 01:51:44PM +0300, Maxim Ostapenko wrote:
>> Ok, got it. The first solution would require changes in libsanitizer because
>> heuristic doesn't work for GCC, so perhaps new UBSan entry point should go
>> upstream, right? Or this may be implemented as local patch for GCC?
> No.  The heuristics relies on:
> 1) either it is old style float cast overflow without location
> 2) or it is new style float cast with location, but the location must:
>     a) not have NULL filename
>     b) the filename must not be ""
>     c) the filename must not be "\1"
> So, my proposal was to emit in GCC the old style float cast overflow if a), b) or
> c) is true, otherwise the new style.  I have no idea what you mean by
> heuristic doesn't work for GCC after that.

I mean that there are some cases where (FilenameOrTypeDescriptor[0] + 
FilenameOrTypeDescriptor[1] < 2) is not sufficient to determine if we 
should use old style. I actually caught this on float-cast-overflow-10.c 
testcase. Here:

$ /home/max/build/master-ref/gcc/xgcc -B/home/max/build/master-ref/gcc/ 
/home/max/workspace/downloads/svn/trunk/gcc/testsuite/c-c++-common/ubsan/float-cast-overflow-10.c 
-B/home/max/build/master-ref/x86_64-unknown-linux-gnu/./libsanitizer/ 
-B/home/max/build/master-ref/x86_64-unknown-linux-gnu/./libsanitizer/ubsan/ 
-L/home/max/build/master-ref/x86_64-unknown-linux-gnu/./libsanitizer/ubsan/.libs 
-fno-diagnostics-show-caret -fdiagnostics-color=never -O2 
-fsanitize=float-cast-overflow -fsanitize-recover=float-cast-overflow 
-DUSE_INT128 -DUSE_DFP -DBROKEN_DECIMAL_INT128 -lm -o 
./float-cast-overflow-10.s -S

$ cat float-cast-overflow-10.s

cvt_sc_d32:
.LFB0:
         .cfi_startproc
         pushq   %rbx
......
.L6:
         movl    %ebx, %esi
         movl    $.Lubsan_data0, %edi
         call    __ubsan_handle_float_cast_overflow
.......
.Lubsan_data0:
         .quad   .Lubsan_type1
         .quad   .Lubsan_type0
         .align 2
         .type   .Lubsan_type1, @object
         .size   .Lubsan_type1, 17
.Lubsan_type1:
         .value  -1  // <- TypeKind
         .value  32
         .string "'_Decimal32'"
         .align 2
         .type   .Lubsan_type0, @object
         .size   .Lubsan_type0, 18
.Lubsan_type0:
         .value  0 // <- TypeKind
         .value  7
         .string "'signed char'"
         .section        .rodata.cst4,"aM",@progbits,4
         .align 4

Here, one can see, we have FilenameOrTypeDescriptor[0]  == -1 and 
FilenameOrTypeDescriptor[1] == 0. So, we end up with wrong decision and 
have SEGV later.
>> BTW, I actually saw UNKNOWN_LOCATION for this expr:
>>
>> volatile double var;  // this is tcc_decaration, so we have UNKNOWN_LOCATION
>> for it.
> This is not a complete testcase, so I wonder what exactly you are talking
> about.  The above doesn't not generate any
> __ubsan_handle_float_cast_overflow calls with
> -fsanitize=float-cast-overflow, and
> volatile double d;
> int bar (void) { return d; }
> has location.
>
> 	Jakub
>

On Wed, Oct 14, 2015 at 03:02:22PM +0300, Maxim Ostapenko wrote:
> On 14/10/15 14:06, Jakub Jelinek wrote:
> >On Wed, Oct 14, 2015 at 01:51:44PM +0300, Maxim Ostapenko wrote:
> >>Ok, got it. The first solution would require changes in libsanitizer because
> >>heuristic doesn't work for GCC, so perhaps new UBSan entry point should go
> >>upstream, right? Or this may be implemented as local patch for GCC?
> >No.  The heuristics relies on:
> >1) either it is old style float cast overflow without location
> >2) or it is new style float cast with location, but the location must:
> >    a) not have NULL filename
> >    b) the filename must not be ""
> >    c) the filename must not be "\1"
> >So, my proposal was to emit in GCC the old style float cast overflow if a), b) or
> >c) is true, otherwise the new style.  I have no idea what you mean by
> >heuristic doesn't work for GCC after that.
> 
> I mean that there are some cases where (FilenameOrTypeDescriptor[0] +
> FilenameOrTypeDescriptor[1] < 2) is not sufficient to determine if we should
> use old style. I actually caught this on float-cast-overflow-10.c testcase.

Ah, ok, in that case the heuristics is flawed.  If they want to keep it,
they should check if MaybeFromTypeKind is either < 2 or equal to 0x1fe.
Can you report it upstream?  If that is changed, we'd need to change the
above and also add
  d) the filename must not start with "\xff\xff"
to the rules.

I think it would be better to just add a whole new entrypoint, but if they
think the heuristics is good enough, they should at least fix it up.

	Jakub