Subject: Re: bug#76393: Warn about -fsanitizer=address builds

From: Pip Cet <[email-masked]>
Date: Wed, 19 Feb 2025 12:18:26 +0000

"Paul Eggert" <[email-masked]> writes:

> On 2/18/25 07:48, Pip Cet wrote:
>>   Building Emacs with address sanitization can help debug memory-use
>> -problems, such as freeing the same object twice.  To use
>> -AddressSanitizer with GCC and similar compilers, append
>> +problems, such as freeing the same object twice.  However,
>> +-fsanitize=address also introduces bugs in the compiled code which cause
>> +GC problems and phantom crashes, which do not always look like they're
>> +caused by the address sanitizer.
>
> I don't see a GCC bug there.

I understand the desire to be diplomatic here, and I'll adjust the patch
accordingly, but I don't agree: fake stacks are a rashly-implemented
manipulation, violating established ABI conventions, which will cause
false positives which are then hard to debug because none of them are
documented sufficiently, and the error messages provide no indication
that the sanitizer is to blame.

> What I see is an incompatibility with Emacs's garbage collector, a
> collector which relies on several aspects of undefined behavior in
> C. So I suggest changing the last three lines to something like this:

The idea that any significant program can rely only on defined behavior
of C is nice, but it's not even close to being true.

> as the default behavior of -fsanitize=address is incompatible with
> ordinary Emacs operation, it needs to be combined with other options.

It is important to point out that these bugs do not mention the
sanitizer in the crash report, and that the only "option" to make GCC
generate correct code is an environment variable, which is error-prone
and inconvenient.

>> +ASAN_OPTIONS='detect_stack_use_after_return=0,detect_leaks=0' in the
>> +environment to suppress diagnostics of minor memory leaks in Emacs, and
>> +buggy code which relocates objects from the stack to the heap .  For
>> +example:
>
> Here, change "buggy code which relocates" to "to suppress relocation
> of".

Again, happy to make the change if the decision is to remain diplomatic
here.  If the option were "no_fake_stack=1", I'd agree.  But the option
isn't called that: there is no ASAN option to insist on a traditional
stack, we can only disable the various "features" which incidentally
violate traditional stack assumptions.  Today, it's
'detect_stack_use_after_return', but there's no guarantee (or
documentation) of that.

> Also, omit the space before ".".

Oops.  I'd written more there :-)

> Otherwise, it looks good.
>
> I've found -fsanitize=address to be verrrrry helpful when debugging
> obscure memory problems, and it's good to document how to use it.

Well, it hasn't worked for a while.

> It would be even better if we changed Emacs so that ASAN_OPTIONS need
> not contain set detect_stack_use_after_return=0, as that would help
> detect more errors. -fsanitize=address has support for this sort of
> thing; see its __asan_addr_is_in_fake_stack function. However, Emacs
> currently doesn't use that support, so we must currently settle for
> detect_stack_use_after_return=0.

__asan_get_current_fake_stack is marked for removal in GCC, and it's not
quite clear whether that comment applies to
__asan_addr_is_in_fake_stack, too.  However, I don't see how either
function would help us detect all the fake stacks and their extents.

There is an established API for split stacks, implemented by
-fsplit-stack (Emacs doesn't support it by default, but I wrote a patch
to add support, and it's not hard).  ASAN doesn't use it, and the
alternatives it provides are unusable, so I'm skeptical that it's a tool
which is worth the significant effort to "support" it.

Pip