[llvm-dev] [cfe-dev] RFC: Implementing -fno-delete-null-pointer-checks in clang

[Sanjoy Das via llvm-dev]

On Thu, Apr 19, 2018 at 3:56 PM, Manoj Gupta via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> My understanding is we only want to disable  the optimizations regarding
> undefined behavior
> related to null pointer deference optimizations. And address space checks
> will end up
> disabling more optimizations than needed.

[Repeating what others have already mentioned on this thread]

I this it is better to avoid framing this as "disable optimizations that exploit
UB" and instead frame this as "define some behavior that was undefined before".

> I did look at some of the optimizations/transforms and there are some that
> we definitely want to keep.
>
> Just a quick example from grepping:
> lib/Transforms/Scalar/LoopIdiomRecognize.cpp
> ...........
>              // Don't create memset_pattern16s with address spaces.
>              StorePtr->getType()->getPointerAddressSpace() == 0 &&
>              (PatternValue = getMemSetPatternValue(StoredVal, DL))) {
>     // It looks like we can use PatternValue!
>     return LegalStoreKind::MemsetPattern;
>   }
>
> Even worse, Sanitizers do NOT work with address spaces which is a big deal
> breaker IMO.

IMO fixing these seems less engineering overhead in the long term than
introducing
yet another knob to the IR.

More importantly, we should _not_ be doing these optimizations without auditing
them individually.  For instance with -fno-delete-null-pointer checks, it isn't
okay to change a memset loop to a call to llvm.memset unless we've ensured llvm
DTRT for llvm.memset and null checks (i.e. the null check in "llvm.memset(ptr,
...); if (!ptr) {}" does not get optimized away).

> Since address spaces and null pointers are really orthogonal issues, I would
> prefer to
> not conflate them.

I'm not sure I agree with this.  Address spaces are a mechanism to provide
additional semantics to pointers.  In this case the additional property we
want is "address 0 may be dereferenceable", and using address spaces seems
appropriate.

> In addition, It is already not easy to convince Linux Kernel maintainers to
> accept clang specific patches.

Maybe I'm missing something, but I thought in this address space scheme we'd
still provide a -fno-delete-null-ptr-checks flag -- it is clang that would mark
pointers with address space n in this mode.

> From Linux kernel maintainers POV, Clang built kernels are already "getting
> lucky". So I am not too
> worried about missing a few cases.
> I'll be glad to fix the missing cases whenever reported.

It seems to me that adding back missing (but correct) optimizations when
reported is better than removing existing (but incorrect) optimizations when
reported.  If I were a kernel developer (which I am not) I'd rather have a
kernel that boots slower than a security vulnerability.

> I also have some other concerns with address spaces e.g. how to pick a safe
> address space not used by
>  any target e.g. many targets (in and out-of-tree) actively use non-zero
> address spaces.
> User code can also specify any address space via
> __attribute__((address_space(N))) so mixing object
> files will be tricky in such cases.

I think for that we can start a thread on cfe-dev and llvm-dev about reserving
address spaces.  While at it, we should reserve a range of address spaces for
LLVM middle-end use so that we can do more things like
-fno-delete-null-pointer-checks without worrying about address space clashes.

-- Sanjoy