[Bug tree-optimization/86732] Potential nullptr dereference does not propagate knowledge about the pointer

2018-08-03 Thread antoshkka at gmail dot com
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86732

Antony Polukhin  changed:

   What|Removed |Added

 Resolution|INVALID |FIXED

--- Comment #9 from Antony Polukhin  ---
(In reply to Jeffrey A. Law from comment #8)
> I wouldn't object to that. In fact I thought we kicked that around along
> with an option to remove path leading to the undefined behavior completely. 
> But it's not something I'm likely to work on.

IMO you will need this flag anyway with the "C++20 Contracts" feature to turn
contract violations into UB/trap/HandlerCall/terminate/...

[Bug tree-optimization/86732] Potential nullptr dereference does not propagate knowledge about the pointer

2018-08-03 Thread law at redhat dot com
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86732

--- Comment #8 from Jeffrey A. Law  ---
I wouldn't object to that. In fact I thought we kicked that around along with
an option to remove path leading to the undefined behavior completely.  But
it's not something I'm likely to work on.

[Bug tree-optimization/86732] Potential nullptr dereference does not propagate knowledge about the pointer

2018-08-02 Thread glisse at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86732

--- Comment #7 from Marc Glisse  ---
(In reply to Jeffrey A. Law from comment #5)
> And FWIW, I think we should be using __builtin_trap rather than
> __builtin_unreachable in many more places because of the security concerns.

It would be better to control it with flags instead of doing it inconsistently
depending on who wrote each pass. We already have -fsanitize=unreachable
-fsanitize-undefined-trap-on-error to automatically replace
__builtin_unreachable with __builtin_trap. However, we do not have an opposite
'performance' option for cases where security is irrelevant.

[Bug tree-optimization/86732] Potential nullptr dereference does not propagate knowledge about the pointer

2018-08-02 Thread law at redhat dot com
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86732

Jeffrey A. Law  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |INVALID

--- Comment #6 from Jeffrey A. Law  ---
Code is working as designed AFAICT.

[Bug tree-optimization/86732] Potential nullptr dereference does not propagate knowledge about the pointer

2018-08-02 Thread law at redhat dot com
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86732

Jeffrey A. Law  changed:

   What|Removed |Added

 CC||law at redhat dot com

--- Comment #5 from Jeffrey A. Law  ---
The code is working as designed.  As noted, we want the dereference to occur so
that programs can catch the signal that occurs as a result of the dereference.

Using __builtin_trap is definitely better from a security standpoint because
the program halts after the undefined behavior -- consider an mmu-less system
that doesn't trap on *0.  If you use __builtin_unreachable you'll just start
executing random code which would be a wonderful attack vector.

And FWIW, I think we should be using __builtin_trap rather than
__builtin_unreachable in many more places because of the security concerns.

[Bug tree-optimization/86732] Potential nullptr dereference does not propagate knowledge about the pointer

2018-07-30 Thread glisse at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86732

--- Comment #4 from Marc Glisse  ---
(In reply to Richard Biener from comment #3)
> note how it doesn't eliminate the actual load which probably causes the 
> odd code-generation.

The code says:

  /* We want the NULL pointer dereference to actually occur so that
 code that wishes to catch the signal can do so.

  /* If we had a NULL pointer dereference, then we want to insert the
 __builtin_trap after the statement, for the other cases we want
 to insert before the statement.  */

Maybe that should be protected by more flags, but it is done on purpose.

> security implications.

Those didn't convince me much at the time though, it is far from the only
transformation that can have such effect, and we are not consistent in using
trap vs unreachable.

[Bug tree-optimization/86732] Potential nullptr dereference does not propagate knowledge about the pointer

2018-07-30 Thread rguenth at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86732

Richard Biener  changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2018-07-30
 CC||law at gcc dot gnu.org,
   ||rguenth at gcc dot gnu.org
 Ever confirmed|0   |1

--- Comment #3 from Richard Biener  ---
We have the isolate-errorneous-paths pass which does:

example (const int & a)
{
  int _3;
  int _4;
  const int * _5;
  const int * _6;
  int _7;

   [local count: 1073741825]:
  _3 = *a_2(D);
  if (_3 == 0)
goto ; [100.00%]
  else
goto ; [0.00%]

   [local count: 598933193]:
  # _5 = PHI 
  _4 = *_5;
  return _4;

   [count: 0]:
  # _6 = PHI <0B(2)>
  _7 ={v} *_6;
  __builtin_trap ();

}

note how it doesn't eliminate the actual load which probably causes the 
odd code-generation.  Nothing removes it afterwards because it may
(and will!) trap.

Then isolate-errorneus-paths runs a bit late so the next CSE pass to
CSE *a_2(D) and *_5 is PRE.  But we still end up with the following
in the end, keeping the condition live.  As Marc said the condition
would be only dead if we'd use __builtin_unreachable () rather than
__builtin_trap () here which has security implications.  But the
load doesn't need to be there.

   [local count: 1073741825]:
  _3 = *a_2(D);
  if (_3 == 0)
goto ; [100.00%]
  else
goto ; [0.00%]

   [local count: 598933193]:
  return _3;

   [count: 0]:
  _7 ={v} MEM[(const int *)0B];
  __builtin_trap ();

[Bug tree-optimization/86732] Potential nullptr dereference does not propagate knowledge about the pointer

2018-07-30 Thread glisse at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86732

--- Comment #2 from Marc Glisse  ---
While I would also like to see this optimized better, ISTR that this was done
on purpose, you may want to look at the old discussions. Some languages may
have things set up to catch null dereferences, but that should be controlled by
-fdelete-null-pointer-checks, -fnon-call-exceptions or some similar flag. Maybe
that was just because Jeff doesn't like __builtin_unreachable (for security
reasons).

[Bug tree-optimization/86732] Potential nullptr dereference does not propagate knowledge about the pointer

2018-07-30 Thread antoshkka at gmail dot com
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86732

--- Comment #1 from Antony Polukhin  ---
Probably a more fair example< without taking an address of a reference:

static const int* get_if(const int* v) {
if (v && *v == 0) return v;
return nullptr;
}

int example(const int* a) {
return *get_if(a);
}

GCC produces the same suboptimal assembly:

_Z7examplePKi:
  test rdi, rdi
  je .L2
  mov eax, DWORD PTR [rdi]
  test eax, eax
  jne .L2
  xor eax, eax
  ret
_Z7examplePKi.cold.0:
.L2:
  mov eax, DWORD PTR ds:0
  ud2


While Clang just generates:
_Z7examplePKi: # @_Z7examplePKi
  mov eax, dword ptr [rdi]
  ret