On 07/05/2018 12:01 PM, Jakub Jelinek wrote:
> On Wed, Jul 04, 2018 at 08:20:47PM +0300, Maxim Ostapenko wrote:
>> On 07/04/2018 05:45 AM, Jeff Law wrote:
>>> On 05/23/2018 11:15 AM, Maxim Ostapenko wrote:
>>>> as described in PR, when using both ASan and UBSan
>>>> (-fsanitize=address,undefined ), we have symbols collision for global
>>>> functions, like __sanitizer_set_report_path. This leads to fuzzy results
>>>> when printing reports into files e.g. for this test case:
>>>>
>>>> #include <sanitizer/common_interface_defs.h>
>>>> int main(int argc, char **argv) {
>>>> __sanitizer_set_report_path("/tmp/sanitizer.txt");
>>>> int i = 23;
>>>> i <<= 32;
>>>> int *array = new int[100];
>>>> delete [] array;
>>>> return array[argc];
>>>> }
>>>>
>>>> only ASan's report gets written to file; UBSan output goes to stderr.
>>>>
>>>> To resolve this issue we could use two approaches:
>>>>
>>>> 1) Use the same approach to that is implemented in Clang (UBSan embedded
>>>> to ASan). The only caveat here is that we need to link (unused) C++ part
>>>> of UBSan even in C programs when linking static ASan runtime. This
>>>> happens because GCC, as opposed to Clang, doesn't split C and C++
>>>> runtimes for sanitizers.
>>>>
>>>> 2) Just add SANITIZER_INTERFACE_ATTRIBUTE to report_file global
>>>> variable. In this case all __sanitizer_set_report_path calls will set
>>>> the same report_file variable. IMHO this is a hacky way to fix the
>>>> issue, it's better to use the first option if possible.
>>>>
>>>>
>>>> The attached patch fixes the symbols collision by embedding UBSan into
>>>> ASan (variant 1), just like we do for LSan.
>>>>
>>>>
>>>> Regtested/bootstrapped on x86_64-unknown-linux-gnu, looks reasonable
>>>> enough for trunk?
>>>>
>>>>
>>>> -Maxim
>>>>
>>>>
>>>> pr84250-2.diff
>>>>
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2018-05-23 Maxim Ostapenko <m.ostape...@samsung.com>
>>>>
>>>> * config/gnu-user.h (LIBASAN_EARLY_SPEC): Pass -lstdc++ for static
>>>> libasan.
>>>> * gcc.c: Do not pass LIBUBSAN_SPEC if ASan is enabled with UBSan.
>>>>
>>>> libsanitizer/ChangeLog:
>>>>
>>>> 2018-05-23 Maxim Ostapenko <m.ostape...@samsung.com>
>>>>
>>>> * Makefile.am: Reorder libs.
>>>> * Makefile.in: Regenerate.
>>>> * asan/Makefile.am: Define DCAN_SANITIZE_UB=1, add dependancy from
>>>> libsanitizer_ubsan.la.
>>>> * asan/Makefile.in: Regenerate.
>>>> * ubsan/Makefile.am: Define new libsanitizer_ubsan.la library.
>>>> * ubsan/Makefile.in: Regenerate.
>>> You know this code better than anyone else working on GCC. My only
>>> concern would be the kernel builds with asan, but I suspect they're
>>> providing their own runtime anyway, so the libstdc++ caveat shouldn't apply.
>> Yes, you are right, kernel provides its own runtime.
>>
>>> OK for the trunk.
>> Ok, thanks, I'll apply the patch today (with fixed ChangeLog entry).
> This broke the c-c++-common/asan/pr59063-2.c test:
>
> FAIL: c-c++-common/asan/pr59063-2.c -O1 (test for excess errors)
> Excess errors:
> /usr/bin/ld: cannot find -lstdc++
I must mis-looked this, sorry :(.
> While it could be fixed by tweaking asan-dg.exp, thinking about this, the
> 1) variant is actually not a good idea, it will not work properly anyway
> if you link one library with -fsanitize=undefined and another library
> with -fsanitize=address, the right solution is to make the two libraries
> coexist sanely
Yes, you're right. Btw, we have pretty the same situation with ASan +
LSan, right?
> , so I'd prefer 2) or if not exporting a variable, export
> an accessor function to get the address of the variable (or whole block of
> shared state in one object between the libraries).
>
> Yes, it means trying to get something accepted upstream, but anything else
> is an ugly hack.
Ok. Could you please revert this patch for me (I don't have a write
access to repo right now) so we can cook a proper fix?
-Maxim
>
> Jakub
>
>
>
