[PATCH] D103009: [DSE] Transform memset + malloc --> calloc (PR25892)

[Dávid Bolvanský via Phabricator via llvm-commits]

xbolva00 added a comment.

In D103009#2955964 <https://reviews.llvm.org/D103009#2955964>, @alexfh wrote:

> In D103009#2954485 <https://reviews.llvm.org/D103009#2954485>, @xbolva00 wrote:
>
>> In D103009#2954430 <https://reviews.llvm.org/D103009#2954430>, @alexfh wrote:
>>
>>> This commit seems to cause memory usage (rss) increase in MariaDB's mysqld by a factor of 4. Looking back into the history, I found that the previous commit here caused the same regression, but we quickly picked up the revert and moved on. Now I'm trying to isolate the problem, but it's taking time.
>>> So far, my sole hypothesis is that malloc + memset of a smaller size can still be converted to calloc. But I have no evidence so far. Naive attempts to synthetically reproduce this didn't work. Maybe this only happens when some UB is in place, but again, I have nothing to back this up.
>>>
>>> Given this is a quite serious regression, can we roll this back while I'm investigating?
>>
>> You should be able to use flag -fno-builtin-calloc to disable this transformation.
>>
>> This transformation is correct and valid; GCC does it as well. No reason to revert, not justified. You should check asm diffs w and w/o patch for any surprises.
>
> I found and reduced a test case that shows a too eager replacement of malloc with calloc:
>
> https://gcc.godbolt.org/z/dTjonof74
>
>   $ cat test.cc
>   #include <stdlib.h>
>   #include <string.h>
>   
>   void *my_malloc(size_t size, int my_flags)
>   {
>     void* point = malloc(size);
>     if (my_flags & 32) memset(point, 0, size);
>     return point;
>   }
>   
>   $ clang -O2 -o test.o -save-temps -c test.cc  && cat test.s
>           .text
>           .file   "test.cc"
>           .globl  _Z9my_mallocmi                  # -- Begin function _Z9my_mallocmi
>           .p2align        4, 0x90
>           .type   _Z9my_mallocmi, at function
>   _Z9my_mallocmi:                         # @_Z9my_mallocmi
>           .cfi_startproc
>   # %bb.0:
>           pushq   %rax
>           .cfi_def_cfa_offset 16
>           movq    %rdi, %rsi
>           movl    $1, %edi
>           callq   calloc at PLT
>           popq    %rcx
>           .cfi_def_cfa_offset 8
>           retq
>   .Lfunc_end0:
>           .size   _Z9my_mallocmi, .Lfunc_end0-_Z9my_mallocmi
>           .cfi_endproc
>                                           # -- End function
>           .ident  "clang version trunk (45ac5f5441818afa1b0ee4a3734583c8cc915a79)"
>           .section        ".note.GNU-stack","", at progbits
>           .addrsig
>
> This is quite a serious problem. Would you please revert while working on a fix?

For now, use -fno-builtin-calloc, it works fine.  Also another solution:

  void *my_malloc(size_t size, int my_flags) __attribute__((no_builtin("calloc")))
  {
    void* point = malloc(size);
    if (my_flags & 32) memset(point, 0, size);
    return point;
  }



>> I guess the easy fix here is to require that `memset` post-dominates the `malloc`,

Yeah, but I am not sure, this is quite very specific case, I would prefer ' __attribute__((no_builtin("calloc")))' solution here, instead of messing with malloc's internals and their impact on LLVM & Langref.

It would be great not to break much more common pattern:

  void* point = malloc(size);
    if (point) memset(point, 0, size);


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103009/new/

https://reviews.llvm.org/D103009