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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
41 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
Hi,

This is regarding support for -fno-delete-null-pointer-checks in clang (PR
9251).
Linux kernel uses this flag to keep null pointer checks from getting
optimized away.  Since clang does not currently support this flag, it often
invites comments from kernel devs that clang is not yet *ready* to compile
Linux kernel.
I have also heard that developers working on firmware, bare-metal tools and
other low level tools often want to use this flag as well.

Therefore, I would like to implement support for this flag (maybe with a
different name), and would like to know the opinions on how it should be
implemented.

Some options/opinions:

1 (From Duncan Sands comment at PR 9251)
> This could be implemented by putting pointers in a non-default address
space when this flag is passed.
Any ideas how will this work for languages/targets that actively make use
of non-default address spaces e.g. OpenCL/GPU targets.  Also, I believe
that allocas still need to kept in default address space so they need to be
bitcast to the *no-default* address space before use.

2. Use this flag like any other optimization flag. Find and fix all
optimizations in clang/llvm related to null pointer accesses.

Thanks,
Manoj

Background:
https://lkml.org/lkml/2018/4/4/601

 From Linus Torvalds:

Note that we explicitly use "-fno-delete-null-pointer-checks" because
we do *not* want NULL pointer check removal even in the case of a bug.

See commit a3ca86aea507 ("Add '-fno-delete-null-pointer-checks' to gcc
CFLAGS") for the reason: we had buggy code that accessed a pointer
before the NULL pointer check, but the bug was "benign" as long as the
compiler didn't actually remove the check.

And note how the buggy code *looked* safe. It was doing the right
check, it was just that the check was hidden and disabled by another
bug.

Removing the NULL pointer check turned a benign bug into a trivially
exploitable one by just mapping user space data at NULL (which avoided
the kernel oops, and then made the kernel use the user value!).

Now, admittedly we have a ton of other security features in place to
avoid these kinds of issues - SMAP helps on the hardware side, and not
allowing users to mmap() NULL in the first place helps with most
distributions, but the point remains: the kernel generally really
doesn't want optimizations that are perhaps allowed by the standard,
but that result in code generation that doesn't match the source code.

The NULL pointer removal is one such thing: don't remove checks in the
kernel based on "standard says". It's ok to do optimizations that are
based on "hardware does the exact same thing", but not on "the
standard says this is undefined so we can remove it".
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On 18 April 2018 at 18:13, Manoj Gupta via cfe-dev
<[hidden email]> wrote:
> Therefore, I would like to implement support for this flag (maybe with a
> different name),

I'd suggest -mdo-what-i-mean; the whole idea is horribly
underspecified, and basically rips up the LangRef in favour of a
nebulous set of good and bad optimizations (probably as dictated by
the ones that have bitten someone who wrote bad code recently and was
grumpy enough about it to complain at us).

In particular I'm skeptical that the address space solution actually
works. What they actually mean is probably not so much about removing
the icmps, but about the control-flow decisions based on them. And
LLVM might not be aware that any given conditional branch comes from a
tainted comparison.

Cheers.

Tim.
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On 4/18/2018 11:21 AM, Tim Northover via cfe-dev wrote:
> On 18 April 2018 at 18:13, Manoj Gupta via cfe-dev
> <[hidden email]> wrote:
>> Therefore, I would like to implement support for this flag (maybe with a
>> different name),
> I'd suggest -mdo-what-i-mean; the whole idea is horribly
> underspecified, and basically rips up the LangRef in favour of a
> nebulous set of good and bad optimizations (probably as dictated by
> the ones that have bitten someone who wrote bad code recently and was
> grumpy enough about it to complain at us).

Despite the name, the flag actually has rather straightforward semantics
from the compiler's perspective.  From the gcc docs for
-fdelete-null-pointer-checks: "Assume that programs cannot safely
dereference null pointers, and that no code or data element resides at
address zero."  (-fno-delete-null-pointer-checks is the opposite.)

-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
> Despite the name, the flag actually has rather straightforward semantics
> from the compiler's perspective.  From the gcc docs for
> -fdelete-null-pointer-checks: "Assume that programs cannot safely
> dereference null pointers, and that no code or data element resides at
> address zero."  (-fno-delete-null-pointer-checks is the opposite.)

Ah, now that's quite a bit more palatable. I really should have read
the docs before spouting "my favourite rant #1". Then my main concern
is that we end up with a sufficiently clear (and documented)
definition that we're not promising more than we intend. I get very
grumpy if I can't tell someone with UB that they're Doing It Wrong.

Of the two options, I still think the second is a non-starter.
Something between the two might be a datalayout flag specifying
addrspace(0) behaviour. It's pretty easy to argue that it'd be good if
code used some kind of
"DataLayout::isPointerIntrinsicallyInvalid(Value *)" for this kind of
thing anyway (rename or relocate at will).

And the name really is terrible, we should change it if at all feasible

Tim.
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
I'm working with an embedded architecture that could, in some situations, run faster if code or data could be located at address zero. I don't know whether this applies to other embedded chips.

Despite the name, the flag actually has rather straightforward semantics
from the compiler's perspective.  From the gcc docs for
-fdelete-null-pointer-checks: "Assume that programs cannot safely
dereference null pointers, and that no code or data element resides at
address zero."  (-fno-delete-null-pointer-checks is the opposite.)

-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On Wed, Apr 18, 2018 at 12:54 PM Tim Northover <[hidden email]> wrote:


On Wed, Apr 18, 2018 at 12:02 PM Friedman, Eli <[hidden email]> wrote: 
> Despite the name, the flag actually has rather straightforward semantics
> from the compiler's perspective.  From the gcc docs for
> -fdelete-null-pointer-checks: "Assume that programs cannot safely
> dereference null pointers, and that no code or data element resides at
> address zero."  (-fno-delete-null-pointer-checks is the opposite.)

Ah, now that's quite a bit more palatable. I really should have read
the docs before spouting "my favourite rant #1". Then my main concern
is that we end up with a sufficiently clear (and documented)
definition that we're not promising more than we intend. I get very
grumpy if I can't tell someone with UB that they're Doing It Wrong.

Of the two options, I still think the second is a non-starter.
Something between the two might be a datalayout flag specifying
addrspace(0) behaviour. It's pretty easy to argue that it'd be good if
code used some kind of
"DataLayout::isPointerIntrinsicallyInvalid(Value *)" for this kind of
thing anyway (rename or relocate at will).

And the name really is terrible, we should change it if at all feasible


On Wed, Apr 18, 2018 at 1:46 PM Jon Chesterfield via llvm-dev <[hidden email]> wrote:
I'm working with an embedded architecture that could, in some situations, run faster if code or data could be located at address zero. I don't know whether this applies to other embedded chips.

Despite the name, the flag actually has rather straightforward semantics
from the compiler's perspective.  From the gcc docs for
-fdelete-null-pointer-checks: "Assume that programs cannot safely
dereference null pointers, and that no code or data element resides at
address zero."  (-fno-delete-null-pointer-checks is the opposite.)

-Eli


 Thanks Tim and Eli,
I should have put the GCC description for the flag in the email.

Regarding the suggestion to DataLayout, It is an interesting idea. I like it in particular since it will avoid creating a new llvm optimization flag and is auto embedded in IR.

Given that, how do we want to proceed, do we want to add yet another field to the DataLayout string?

e.g. 
1. Add to already overloaded pointer types? e.g. p[n]:<size>:<abi>:<pref>:<idx>: <NullPointerValid ? 1 : 0>
2. (My preference) Or create another unique field identifier: e.g. np[n]: <NullPointerValid ? 1 : 0> 

Thanks,
Manoj

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On 4/19/2018 11:48 AM, Manoj Gupta via llvm-dev wrote:
On Wed, Apr 18, 2018 at 12:54 PM Tim Northover <[hidden email]> wrote:


On Wed, Apr 18, 2018 at 12:02 PM Friedman, Eli <[hidden email]> wrote: 
> Despite the name, the flag actually has rather straightforward semantics
> from the compiler's perspective.  From the gcc docs for
> -fdelete-null-pointer-checks: "Assume that programs cannot safely
> dereference null pointers, and that no code or data element resides at
> address zero."  (-fno-delete-null-pointer-checks is the opposite.)

Ah, now that's quite a bit more palatable. I really should have read
the docs before spouting "my favourite rant #1". Then my main concern
is that we end up with a sufficiently clear (and documented)
definition that we're not promising more than we intend. I get very
grumpy if I can't tell someone with UB that they're Doing It Wrong.

Of the two options, I still think the second is a non-starter.
Something between the two might be a datalayout flag specifying
addrspace(0) behaviour. It's pretty easy to argue that it'd be good if
code used some kind of
"DataLayout::isPointerIntrinsicallyInvalid(Value *)" for this kind of
thing anyway (rename or relocate at will).

And the name really is terrible, we should change it if at all feasible


On Wed, Apr 18, 2018 at 1:46 PM Jon Chesterfield via llvm-dev <[hidden email]> wrote:
I'm working with an embedded architecture that could, in some situations, run faster if code or data could be located at address zero. I don't know whether this applies to other embedded chips.

Despite the name, the flag actually has rather straightforward semantics
from the compiler's perspective.  From the gcc docs for
-fdelete-null-pointer-checks: "Assume that programs cannot safely
dereference null pointers, and that no code or data element resides at
address zero."  (-fno-delete-null-pointer-checks is the opposite.)

-Eli


 Thanks Tim and Eli,
I should have put the GCC description for the flag in the email.

Regarding the suggestion to DataLayout, It is an interesting idea. I like it in particular since it will avoid creating a new llvm optimization flag and is auto embedded in IR.

Given that, how do we want to proceed, do we want to add yet another field to the DataLayout string?

Modifying the datalayout is not a good idea; it doesn't interact with LTO correctly, and the frontend and the backend generally need to agree on the datalayout.

You could maybe use a module flag, if the address-space thing doesn't work for some reason, but we don't like to introduce more of those if we can avoid it.

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On 4/19/2018 11:57 AM, Friedman, Eli via cfe-dev wrote:
On 4/19/2018 11:48 AM, Manoj Gupta via llvm-dev wrote:
On Wed, Apr 18, 2018 at 12:54 PM Tim Northover <[hidden email]> wrote:


On Wed, Apr 18, 2018 at 12:02 PM Friedman, Eli <[hidden email]> wrote: 
> Despite the name, the flag actually has rather straightforward semantics
> from the compiler's perspective.  From the gcc docs for
> -fdelete-null-pointer-checks: "Assume that programs cannot safely
> dereference null pointers, and that no code or data element resides at
> address zero."  (-fno-delete-null-pointer-checks is the opposite.)

Ah, now that's quite a bit more palatable. I really should have read
the docs before spouting "my favourite rant #1". Then my main concern
is that we end up with a sufficiently clear (and documented)
definition that we're not promising more than we intend. I get very
grumpy if I can't tell someone with UB that they're Doing It Wrong.

Of the two options, I still think the second is a non-starter.
Something between the two might be a datalayout flag specifying
addrspace(0) behaviour. It's pretty easy to argue that it'd be good if
code used some kind of
"DataLayout::isPointerIntrinsicallyInvalid(Value *)" for this kind of
thing anyway (rename or relocate at will).

And the name really is terrible, we should change it if at all feasible


On Wed, Apr 18, 2018 at 1:46 PM Jon Chesterfield via llvm-dev <[hidden email]> wrote:
I'm working with an embedded architecture that could, in some situations, run faster if code or data could be located at address zero. I don't know whether this applies to other embedded chips.

Despite the name, the flag actually has rather straightforward semantics
from the compiler's perspective.  From the gcc docs for
-fdelete-null-pointer-checks: "Assume that programs cannot safely
dereference null pointers, and that no code or data element resides at
address zero."  (-fno-delete-null-pointer-checks is the opposite.)

-Eli


 Thanks Tim and Eli,
I should have put the GCC description for the flag in the email.

Regarding the suggestion to DataLayout, It is an interesting idea. I like it in particular since it will avoid creating a new llvm optimization flag and is auto embedded in IR.

Given that, how do we want to proceed, do we want to add yet another field to the DataLayout string?

Modifying the datalayout is not a good idea; it doesn't interact with LTO correctly, and the frontend and the backend generally need to agree on the datalayout.

You could maybe use a module flag, if the address-space thing doesn't work for some reason, but we don't like to introduce more of those if we can avoid it.

Actually, thinking about it a bit more, a function attribute would be better than a module flag.  But I'd still like to explore the address-space thing first, since we already have the LLVM infrastructure to make that work.

-Eli
-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
On 19 April 2018 at 19:57, Friedman, Eli via cfe-dev
<[hidden email]> wrote:
> Modifying the datalayout is not a good idea; it doesn't interact with LTO
> correctly, and the frontend and the backend generally need to agree on the
> datalayout.

Probably a fair point, though I think it has a subtle effect on how
the feature is documented. It becomes more about whether code in this
translation unit is permitted to access an object at 0 than whether
such an object exists.

Cheers.

Tim.
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev


On Thu, Apr 19, 2018 at 11:59 AM Friedman, Eli <[hidden email]> wrote:
On 4/19/2018 11:57 AM, Friedman, Eli via cfe-dev wrote:
On 4/19/2018 11:48 AM, Manoj Gupta via llvm-dev wrote:
On Wed, Apr 18, 2018 at 12:54 PM Tim Northover <[hidden email]> wrote:


On Wed, Apr 18, 2018 at 12:02 PM Friedman, Eli <[hidden email]> wrote: 
> Despite the name, the flag actually has rather straightforward semantics
> from the compiler's perspective.  From the gcc docs for
> -fdelete-null-pointer-checks: "Assume that programs cannot safely
> dereference null pointers, and that no code or data element resides at
> address zero."  (-fno-delete-null-pointer-checks is the opposite.)

Ah, now that's quite a bit more palatable. I really should have read
the docs before spouting "my favourite rant #1". Then my main concern
is that we end up with a sufficiently clear (and documented)
definition that we're not promising more than we intend. I get very
grumpy if I can't tell someone with UB that they're Doing It Wrong.

Of the two options, I still think the second is a non-starter.
Something between the two might be a datalayout flag specifying
addrspace(0) behaviour. It's pretty easy to argue that it'd be good if
code used some kind of
"DataLayout::isPointerIntrinsicallyInvalid(Value *)" for this kind of
thing anyway (rename or relocate at will).

And the name really is terrible, we should change it if at all feasible


On Wed, Apr 18, 2018 at 1:46 PM Jon Chesterfield via llvm-dev <[hidden email]> wrote:
I'm working with an embedded architecture that could, in some situations, run faster if code or data could be located at address zero. I don't know whether this applies to other embedded chips.

Despite the name, the flag actually has rather straightforward semantics
from the compiler's perspective.  From the gcc docs for
-fdelete-null-pointer-checks: "Assume that programs cannot safely
dereference null pointers, and that no code or data element resides at
address zero."  (-fno-delete-null-pointer-checks is the opposite.)

-Eli


 Thanks Tim and Eli,
I should have put the GCC description for the flag in the email.

Regarding the suggestion to DataLayout, It is an interesting idea. I like it in particular since it will avoid creating a new llvm optimization flag and is auto embedded in IR.

Given that, how do we want to proceed, do we want to add yet another field to the DataLayout string?

Modifying the datalayout is not a good idea; it doesn't interact with LTO correctly, and the frontend and the backend generally need to agree on the datalayout.

You could maybe use a module flag, if the address-space thing doesn't work for some reason, but we don't like to introduce more of those if we can avoid it.

Actually, thinking about it a bit more, a function attribute would be better than a module flag.  But I'd still like to explore the address-space thing first, since we already have the LLVM infrastructure to make that work.

-Eli

Thanks Eli,

I was looking around for the cases where AddrSpace !=0 are checked. Seems like there are a bunch of optimizations that will fail to apply for non zero address spaces.
So I'll start with the function attribute approach.

-Manoj



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On 19 April 2018 at 22:36, Manoj Gupta via llvm-dev
<[hidden email]> wrote:
> I was looking around for the cases where AddrSpace !=0 are checked. Seems
> like there are a bunch of optimizations that will fail to apply for non zero
> address spaces.

Isn't that exactly what we want? Did you look in enough detail to
determine that these optimizations *should* have applied? I'd be
pretty interested to know what we disable for the others, because the
the null thing is the only guarantee I knew about that we made.

Also, I'm pretty unconvinced optimization should be our first priority
here. That smacks of the untenable (IMO) option 2. Whatever we do I
want a program with well-defined semantics at the end. Starting from a
known good position and enabling desirable optimizations that we have
decided are valid is a significantly better path than starting with
"you might get lucky" and disabling extra optimizations that are
reported to us.

I realise this actually argues against my datalayout suggestion too,
so I'm getting a lot more convinced by Eli (& Duncan).

Cheers.

Tim.
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev


On Thu, Apr 19, 2018 at 2:53 PM Tim Northover <[hidden email]> wrote:
On 19 April 2018 at 22:36, Manoj Gupta via llvm-dev
<[hidden email]> wrote:
> I was looking around for the cases where AddrSpace !=0 are checked. Seems
> like there are a bunch of optimizations that will fail to apply for non zero
> address spaces.

Isn't that exactly what we want? Did you look in enough detail to
determine that these optimizations *should* have applied? I'd be
pretty interested to know what we disable for the others, because the
the null thing is the only guarantee I knew about that we made.

Also, I'm pretty unconvinced optimization should be our first priority
here. That smacks of the untenable (IMO) option 2. Whatever we do I
want a program with well-defined semantics at the end. Starting from a
known good position and enabling desirable optimizations that we have
decided are valid is a significantly better path than starting with
"you might get lucky" and disabling extra optimizations that are
reported to us.

I realise this actually argues against my datalayout suggestion too,
so I'm getting a lot more convinced by Eli (& Duncan).

Cheers.

Tim.

Thanks a lot Tim,

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.
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.

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

In addition, It is already not easy to convince Linux Kernel maintainers to accept clang specific patches.
Worse performance when compared to GCC may make it even harder to push more patches.
(There are already many complains about clang not supporting optimizations that Linux kernel is used to.
As a side note: x86 maintainers deliberately broke clang support in upstream (https://lkml.org/lkml/2018/4/2/115)
related to asm-goto support. It would be greatly preferable to avoid another confrontation related to performance).

> Starting from a known good position and enabling desirable optimizations that we have
> decided are valid is a significantly better path than starting with "you might get lucky"
> and disabling extra optimizations that are reported to us.

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.

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 hope that I have made the case for not using address spaces. Not that I am against address spaces but
losing performance and sanitizers support is a deal breaker.

Thanks
-Manoj

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev

On 04/18/2018 02:54 PM, Tim Northover via llvm-dev wrote:

>> Despite the name, the flag actually has rather straightforward semantics
>> from the compiler's perspective.  From the gcc docs for
>> -fdelete-null-pointer-checks: "Assume that programs cannot safely
>> dereference null pointers, and that no code or data element resides at
>> address zero."  (-fno-delete-null-pointer-checks is the opposite.)
> Ah, now that's quite a bit more palatable. I really should have read
> the docs before spouting "my favourite rant #1". Then my main concern
> is that we end up with a sufficiently clear (and documented)
> definition that we're not promising more than we intend. I get very
> grumpy if I can't tell someone with UB that they're Doing It Wrong.
>
> Of the two options, I still think the second is a non-starter.
> Something between the two might be a datalayout flag specifying
> addrspace(0) behaviour.

But then we're back to auditing everything (FWIW, I'm not sure there's
going to be a great alternative to an audit). What behavior would you
like under LTO? We could use a different address space by default
(although, unless you also change the address space used for allocas,
you're going to end up with addressspace casts all over the place).

 -Hal

>  It's pretty easy to argue that it'd be good if
> code used some kind of
> "DataLayout::isPointerIntrinsicallyInvalid(Value *)" for this kind of
> thing anyway (rename or relocate at will).
>
> And the name really is terrible, we should change it if at all feasible
>
> Tim.
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
On Wed, Apr 18, 2018 at 3:54 PM Tim Northover via llvm-dev <[hidden email]> wrote:
> Despite the name, the flag actually has rather straightforward semantics
> from the compiler's perspective.  From the gcc docs for
> -fdelete-null-pointer-checks: "Assume that programs cannot safely
> dereference null pointers, and that no code or data element resides at
> address zero."  (-fno-delete-null-pointer-checks is the opposite.)

Ah, now that's quite a bit more palatable. I really should have read
the docs before spouting "my favourite rant #1". Then my main concern
is that we end up with a sufficiently clear (and documented)
definition that we're not promising more than we intend. I get very
grumpy if I can't tell someone with UB that they're Doing It Wrong

+1 for implementing the feature.
 
Of the two options, I still think the second is a non-starter.
Something between the two might be a datalayout flag specifying
addrspace(0) behaviour. It's pretty easy to argue that it'd be good if
code used some kind of
"DataLayout::isPointerIntrinsicallyInvalid(Value *)" for this kind of
thing anyway (rename or relocate at will)

Whether or not this is put into DataLayout, moving all the null-related addrSpace != 0 checks into an accessor seems like a great idea. Besides this feature request, presumably there's also other uses of non-zero address spaces for which the pointer 0 could usefully be treated as invalid...

And the name really is terrible, we should change it if at all feasible

Yep. "-fnull-pointer-is-valid" has been suggested before.


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On 4/20/18, James Y Knight wrote:
>
>
> Yep. "-fnull-pointer-is-valid" has been suggested before.
>

-fplacate-linux-kernel-developers ?

Csaba
--
You can get very substantial performance improvements by not doing the
right thing.
   - Scott Meyers, An Effective C++11/14 Sampler
So if you're looking for a completely portable, 100% standards-conformat way
to get the wrong information: this is what you want. - Scott Meyers
(C++TDaWYK)
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On 20 Apr 2018, at 10:06, Csaba Raduly via cfe-dev <[hidden email]> wrote:
>
> On 4/20/18, James Y Knight wrote:
>>
>>
>> Yep. "-fnull-pointer-is-valid" has been suggested before.
>>
>
> -fplacate-linux-kernel-developers ?

-std=vaguely-like-c99?

David

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev


> On Apr 20, 2018, at 2:06 AM, Csaba Raduly via cfe-dev <[hidden email]> wrote:
>
> On 4/20/18, James Y Knight wrote:
>>
>>
>> Yep. "-fnull-pointer-is-valid" has been suggested before.
>>
>
> -fplacate-linux-kernel-developers ?

Please, lets keep this discussion on topic and productive.  The semantics described upstream of (the inverse of) "Assume that programs cannot safely dereference null pointers, and that no code or data element resides at address zero.” is entirely reasonable.

-Chris

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On 21 Apr 2018, at 22:19, Chris Lattner via llvm-dev <[hidden email]> wrote:

>
>>
>> On Apr 20, 2018, at 2:06 AM, Csaba Raduly via cfe-dev <[hidden email]> wrote:
>>
>> On 4/20/18, James Y Knight wrote:
>>>
>>>
>>> Yep. "-fnull-pointer-is-valid" has been suggested before.
>>>
>>
>> -fplacate-linux-kernel-developers ?
>
> Please, lets keep this discussion on topic and productive.  The semantics described upstream of (the inverse of) "Assume that programs cannot safely dereference null pointers, and that no code or data element resides at address zero.” is entirely reasonable.

I disagree, because it depends on what you mean by dereference.  If it is safe to dereference NULL, then that means that it is also safe to hoist the null dereference above the check.  For example:

        if (x == NULL)
                return;
        y = *x;

Is safe to transform into:

        y = *x;
        if (x == NULL)
                return;

The load is guaranteed not to trap by the fact that NULL a dereferencable address.

As I understand it, the issue in GCC was that they treated the address calculation as a dereference.  The word ‘dereference’ does not appear in the C standard, which talks only about use, and therefore treats *x and &x->y both as uses.  This is intentional, because on segmented architectures it is possible that NULL + X will give you an out-of-bounds and unrepresentable pointer, possibly even causing a trap.

A Linux kernel compiled with clang was not vulnerable, because LLVM was not eliding this check, which makes this flag somewhat pointless.  LLVM assumed that a reachable load or a store of an pointer guaranteed that it was not NULL, but that a GEP did not.

I believe that the correct fix is to explicitly permit GEPs with a NULL base in the LLVM IR spec.  As I understand it, this is already implicitly permitted because code that predates __builtin_offsetof uses a cast of 0 to a struct type and takes the address of a field to get the offset.

Any code that is loading or storing from null prior to a null check probably doesn’t mind if the null check is then elided, because it’s going to trap anyway (supporting C code that allows loads and stores from an address with a bit pattern of 0 when interpreted as an integer is incredibly hard, as our friends at IBM can attest).  

David

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On Sun, Apr 22, 2018, 5:11 AM David Chisnall via cfe-dev <[hidden email]> wrote:
I disagree, because it depends on what you mean by dereference.  If it is safe to dereference NULL, then that means that it is also safe to hoist the null dereference above the check.  For example:

Nope. The intent is that NULL is potentially dereferenceable, just as any other address would be. Not that it's known to always be dereferenceable.

        if (x == NULL)
                return;
        y = *x;

Is safe to transform into:

        y = *x;
        if (x == NULL)
                return;

The load is guaranteed not to trap by the fact that NULL a dereferencable address.

That transform is still not safe with the new flag, because you do not know that address 0 is dereferenceable in that code. You also don't know that it's definitely _not_ dereferenceable, however -- which is the new behavior.
 
Any code that is loading or storing from null prior to a null check probably doesn’t mind if the null check is then elided, because it’s going to trap anyway (supporting C code that allows loads and stores from an address with a bit pattern of 0 when interpreted as an integer is incredibly hard, as our friends at IBM can attest).  

No -- the kernel does mind exactly this, because in some circumstances, dereferencing null does not trap in kernel context. So you end up with both no trap and no check, and a security vulnerability. That's less true now, with the various other protections e.g. min_mmap_address sysctl, SMAP, and others, but still, the desire is to keep this from happening.

Here is the commit which started using the flag in the kernel:

commit a3ca86aea507904148870946d599e07a340b39bf

Author: Eugene Teo <[hidden email]>

Date:   Wed Jul 15 14:59:10 2009 +0800


    Add '-fno-delete-null-pointer-checks' to gcc CFLAGS

    

    Turning on this flag could prevent the compiler from optimising away

    some "useless" checks for null pointers.  Such bugs can sometimes become

    exploitable at compile time because of the -O2 optimisation.

    

    See http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Optimize-Options.html

    

    An example that clearly shows this 'problem' is commit 6bf67672.

    

     static void __devexit agnx_pci_remove(struct pci_dev *pdev)

     {

         struct ieee80211_hw *dev = pci_get_drvdata(pdev);

    -    struct agnx_priv *priv = dev->priv;

    +    struct agnx_priv *priv;

         AGNX_TRACE;

    

         if (!dev)

             return;

    +    priv = dev->priv;

    

    By reverting this patch, and compile it with and without

    -fno-delete-null-pointer-checks flag, we can see that the check for dev

    is compiled away.

    

        call    printk  #

    -   testq   %r12, %r12  # dev

    -   je  .L94    #,

        movq    %r12, %rdi  # dev,

    

    Clearly the 'fix' is to stop using dev before it is tested, but building

    with -fno-delete-null-pointer-checks flag at least makes it harder to

    abuse.

                                                                                                                                Signed-off-by: Eugene Teo <[hidden email]>

    Acked-by: Eric Paris <[hidden email]>

    Acked-by: Wang Cong <[hidden email]>

    Signed-off-by: Linus Torvalds <[hidden email]>


 

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
> In addition, It is already not easy to convince Linux Kernel maintainers to
> accept clang specific patches.
> Worse performance when compared to GCC may make it even harder to push more
> patches.
> (There are already many complains about clang not supporting optimizations
> that Linux kernel is used to.
> As a side note: x86 maintainers deliberately broke clang support in upstream
> (https://lkml.org/lkml/2018/4/2/115)

Yeah, elements of the Linux community seem actively hostile to Clang.
We shouldn't let their hostility dictate our technical policy.

> I hope that I have made the case for not using address spaces.

You've certainly argued against them. You haven't provided adequate
semantics for IR without them though.

Cheers.

Tim.
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
123