Thread overview
[Bug 207] _d_throw is not treated as `noreturn`.
Jan 09, 2016
Iain Buclaw
Jan 09, 2016
Iain Buclaw
Jan 09, 2016
Iain Buclaw
Jan 09, 2016
Iain Buclaw
Jan 09, 2016
Iain Buclaw
Jan 09, 2016
Iain Buclaw
Jan 09, 2016
Artur Skawina
January 09, 2016
http://bugzilla.gdcproject.org/show_bug.cgi?id=207

--- Comment #1 from Iain Buclaw <ibuclaw@gdcproject.org> ---
It should be the same as calling a function which noreturn attribute set.

-- 
You are receiving this mail because:
You are watching all bug changes.


January 09, 2016
http://bugzilla.gdcproject.org/show_bug.cgi?id=207

--- Comment #2 from Iain Buclaw <ibuclaw@gdcproject.org> ---
I mean, s/which/with the/.  Autocorrekt fails again. :-)

-- 
You are receiving this mail because:
You are watching all bug changes.


January 09, 2016
http://bugzilla.gdcproject.org/show_bug.cgi?id=207

art.08.09@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |art.08.09@gmail.com

--- Comment #3 from art.08.09@gmail.com ---
(In reply to Iain Buclaw from comment #1)
> It should be the same as calling a function which noreturn attribute set.

Hence $summary ;)

I was surprised that this was not already the case, and by the cost (I was playing with the new arithmetic builtins and the impact on calling/surrounding code was significant; the dead post-throw code emitted isn't that much of a problem).

BTW, there's a similar issue w/ D's asserts (_d_assert) in
non-release builds.
But there, I'm not sure if assuming no return is legal (no
spec is fun...). Still, I ended up using:

   { assert (0); import gcc.builtins; __builtin_trap(); }

instead of `assert(0)`. At least for the `assert(0)` case
emitting that trap unconditionally seems OK (it's what
happens in release-mode anyway, so recovery is already
impossible).

-- 
You are receiving this mail because:
You are watching all bug changes.


January 09, 2016
http://bugzilla.gdcproject.org/show_bug.cgi?id=207

--- Comment #4 from Iain Buclaw <ibuclaw@gdcproject.org> ---
Yes, I'd imagine you'd see the same, as asserts and bounds error functions are marked in the same way as D throw function.

-- 
You are receiving this mail because:
You are watching all bug changes.


January 09, 2016
http://bugzilla.gdcproject.org/show_bug.cgi?id=207

--- Comment #5 from Iain Buclaw <ibuclaw@gdcproject.org> ---
As for assert(0) in non-release code, it is still possible to catch it, so
there's not much in the way of optimising around that. :-)

I can have a look, but as far as I recall, gcc optimizer should treat it as noreturn.  Whether or not noreturn implies that everything in scope afterwards is unreachable depends the optimizer too.

-- 
You are receiving this mail because:
You are watching all bug changes.


January 09, 2016
http://bugzilla.gdcproject.org/show_bug.cgi?id=207

--- Comment #6 from art.08.09@gmail.com ---
(In reply to Iain Buclaw from comment #5)
> As for assert(0) in non-release code, it is still possible to catch it, so
> there's not much in the way of optimising around that. :-)

It's not about catching -- marking it as noreturn doesn't affect catching, just return/recovery. `assert(0)` is sufficiently special in D that the compiler already complains when it finds reachable code following such an assert:

   void f() {
      assert(0);
      f();       // warning: statement is not reachable
   }


> I can have a look, but as far as I recall, gcc optimizer should treat it as noreturn.  Whether or not noreturn implies that everything in scope afterwards is unreachable depends the optimizer too.

My guess is the calls are not treated as @noreturn, but, since gdc dropped support for most gcc attributes, verifying this by wrapping the calls is currently impossible...

-- 
You are receiving this mail because:
You are watching all bug changes.


January 09, 2016
http://bugzilla.gdcproject.org/show_bug.cgi?id=207

--- Comment #7 from Iain Buclaw <ibuclaw@gdcproject.org> ---
OK, looks like it wasn't being set because of a (silently changed?) behaviour I never anticipated when assigning to bitfields in C/C++.

unsigned volatile_flag : 1;

volatile_flag = (flags & 2);  // Assigning bits are '10', however the
                              // '1' is dropped because it doesn't fit.

I'm sure that this used to work in an earlier version, however I'm even more surprised that this didn't trigger an overflow warning (it should have)!

-- 
You are receiving this mail because:
You are watching all bug changes.


January 09, 2016
http://bugzilla.gdcproject.org/show_bug.cgi?id=207

Iain Buclaw <ibuclaw@gdcproject.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Iain Buclaw <ibuclaw@gdcproject.org> ---
Should be fixed here: https://github.com/D-Programming-GDC/GDC/commit/7244b1c2a2c8b0bc137755b56be2cea93f7ec922

-- 
You are receiving this mail because:
You are watching all bug changes.


January 09, 2016
On 01/09/16 16:09,  via D.gnu wrote:
>  Iain Buclaw <mailto:ibuclaw@gdcproject.org>  changed bug 207 <http://bugzilla.gdcproject.org/show_bug.cgi?id=207>
> What 	Removed 	Added
> Status 	NEW 	RESOLVED
> Resolution 	--- 	FIXED
> 
> *Comment # 8 <http://bugzilla.gdcproject.org/show_bug.cgi?id=207#c8> on bug 207 <http://bugzilla.gdcproject.org/show_bug.cgi?id=207> from Iain Buclaw <mailto:ibuclaw@gdcproject.org> *
> 
> Should be fixed here: https://github.com/D-Programming-GDC/GDC/commit/7244b1c2a2c8b0bc137755b56be2cea93f7ec922

Confirmed. The initial testcase looked like this morning:

0000000000403e40 <_Dmain>:
  403e40:       53                      push   %rbx
  403e41:       89 fb                   mov    %edi,%ebx
  403e43:       01 fb                   add    %edi,%ebx
  403e45:       70 14                   jo     403e5b <_Dmain+0x1b>
  403e47:       81 eb 00 00 00 80       sub    $0x80000000,%ebx
  403e4d:       0f 90 c0                seto   %al
  403e50:       0f b6 d0                movzbl %al,%edx
  403e53:       85 d2                   test   %edx,%edx
  403e55:       75 29                   jne    403e80 <_Dmain+0x40>
  403e57:       89 d8                   mov    %ebx,%eax
  403e59:       5b                      pop    %rbx
  403e5a:       c3                      retq
  403e5b:       bf a0 39 45 00          mov    $0x4539a0,%edi
  403e60:       e8 9b e5 00 00          callq  412400 <_d_throw>
  403e65:       89 d9                   mov    %ebx,%ecx
  403e67:       31 f6                   xor    %esi,%esi
  403e69:       81 e9 00 00 00 80       sub    $0x80000000,%ecx
  403e6f:       40 0f 90 c6             seto   %sil
  403e73:       89 cb                   mov    %ecx,%ebx
  403e75:       85 f6                   test   %esi,%esi
  403e77:       74 11                   je     403e8a <_Dmain+0x4a>
  403e79:       0f 1f 80 00 00 00 00    nopl   0x0(%rax)
  403e80:       bf a0 39 45 00          mov    $0x4539a0,%edi
  403e85:       e8 76 e5 00 00          callq  412400 <_d_throw>
  403e8a:       89 d8                   mov    %ebx,%eax
  403e8c:       5b                      pop    %rbx
  403e8d:       c3                      retq

and now, with the only difference being a new gdc build w/ that commit (cherry-picked onto gcc5 branch), has changed to this:

0000000000413530 <_Dmain>:
  413530:       48 83 ec 08             sub    $0x8,%rsp
  413534:       01 ff                   add    %edi,%edi
  413536:       70 0e                   jo     413546 <_Dmain+0x16>
  413538:       89 f8                   mov    %edi,%eax
  41353a:       2d 00 00 00 80          sub    $0x80000000,%eax
  41353f:       70 05                   jo     413546 <_Dmain+0x16>
  413541:       48 83 c4 08             add    $0x8,%rsp
  413545:       c3                      retq
  413546:       bf e0 f7 44 00          mov    $0x44f7e0,%edi
  41354b:       e8 60 de 00 00          callq  4213b0 <_d_throw>

[Which is the same code as w/ the trailing __builtin_unreachable().]


Thanks.

artur