Message ID | 561CE97C.50002@partner.samsung.com |
---|---|
State | New |
Headers | show |
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
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;