www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Consensus on goto's into catch blocks

reply "Iain Buclaw" <ibuclaw ubuntu.com> writes:
Can someone remind me again what was last agreed when I brought 
this up?

I seem to recall that this should be disallowed as is practically 
always a bug, also, and it skips any initialisation of the 
exception object. (See: http://dlang.org/statement.html - "It is 
illegal for a GotoStatement to be used to skip initializations.")

Current failing test I want to have removed from the test suite.

test/runnable/eh.d:
void test8()
{
   int a;
   goto L2;    // gdc Error: cannot goto into catch block

   try {
       a += 2;
   }
   catch (Exception e) {
       a += 3;
L2: ;
       a += 100;
   }
   assert(a == 100);
}


Thanks
Iain.
Jun 13 2013
next sibling parent reply "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Thu, Jun 13, 2013 at 07:35:39PM +0200, Iain Buclaw wrote:

 Can someone remind me again what was last agreed when I brought this
 up?
 
 I seem to recall that this should be disallowed as is practically
 always a bug, also, and it skips any initialisation of the exception
 object. (See: http://dlang.org/statement.html - "It is illegal for a
 GotoStatement to be used to skip initializations.")
 
 Current failing test I want to have removed from the test suite.
 
 test/runnable/eh.d:
 void test8()
 {
   int a;
   goto L2;    // gdc Error: cannot goto into catch block
 
   try {
       a += 2;
   }
   catch (Exception e) {
       a += 3;
 L2: ;
       a += 100;
   }
   assert(a == 100);
 }


[...]

Hmph, how did this test end up in test/runnable? What if after L2, we
try to deference e? I can't see any way this code can even be remotely
valid. How did this end up in the test suite?? (Or is this supposed to
test whether the compiler is able to ascertain that e is not referenced
after L2? Does DMD even do that kind of analysis?)


T

-- 
"Holy war is an oxymoron." -- Lazarus Long
Jun 13 2013
parent "Iain Buclaw" <ibuclaw ubuntu.com> writes:
On Thursday, 13 June 2013 at 17:51:51 UTC, H. S. Teoh wrote:

 On Thu, Jun 13, 2013 at 07:35:39PM +0200, Iain Buclaw wrote:

 Can someone remind me again what was last agreed when I 
 brought this
 up?
 
 I seem to recall that this should be disallowed as is 
 practically
 always a bug, also, and it skips any initialisation of the 
 exception
 object. (See: http://dlang.org/statement.html - "It is illegal 
 for a
 GotoStatement to be used to skip initializations.")
 
 Current failing test I want to have removed from the test 
 suite.
 
 test/runnable/eh.d:
 void test8()
 {
   int a;
   goto L2;    // gdc Error: cannot goto into catch block
 
   try {
       a += 2;
   }
   catch (Exception e) {
       a += 3;
 L2: ;
       a += 100;
   }
   assert(a == 100);
 }


 [...]

 Hmph, how did this test end up in test/runnable? What if after 
 L2, we
 try to deference e? I can't see any way this code can even be 
 remotely
 valid. How did this end up in the test suite?? (Or is this 
 supposed to
 test whether the compiler is able to ascertain that e is not 
 referenced
 after L2? Does DMD even do that kind of analysis?)



I think the test is for the analysis of the try block.  The 
frontend says: "Right, this try block will not throw, so all 
catches can be removed.", or "This try body is empty, so this 
entire try/catch statement can be removed"  -  However this is 
plain wrong in that the backend will require to know the bodies 
of all the catches in case they are used in some way.

In GDC's case, the compiler will ICE if it finds a goto to a 
label that doesn't exist, whereas DMD will happily accept this as 
valid and instead die when you try to run the program (oops!).  
So since around pre-2.060 I have always #ifdef'd out this 
frontend optimisation.  But since then analysis has been improved 
somewhat, but still misses this edge case.

void test8()
{
   int a;
   goto L2;    // BOOM!

   try { }
   catch (Exception e) {
L2: ;
       a += 100;
   }
   assert(a == 100);
}

But I have made alterations to fix that. :-)


Where GDC also differs from DMD is that the glue-layer part of 
the front-end also has a rudimentary goto/label analyser that 
makes sure that you:
- Can't goto into a try block (though testing just now, the try 
block requires a throw statement, so the frontend must wrongly 
unravel the try {} statement somehow).

- Can't goto into a catch block

- Can't goto into or out of a finally block.

- Can't have case/default in switches nested inside a try block,


Future improvements will be to also include skipped 
initialisations.

{
     goto L2;
     Object o = new Object;

   L2:
     writeln (o);
}

This also has the benefit of fleshing out these sorts of bugs in 
phobos (there are a few, I've already pre-tested it).

(I'm stalling)

But back to the point, to make this analysis work and be valid, 
the front-end must either stop unraveling/removing try/catch 
blocks, or improve the way it analyses code before doing this 
sort of optimisation.   I'm sure Walter would prefer the latter 
with patches welcome.  :o)

Regards
Iain
Jun 13 2013
prev sibling next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Thursday, 13 June 2013 at 17:35:41 UTC, Iain Buclaw wrote:

 Can someone remind me again what was last agreed when I brought 
 this up?

 I seem to recall that this should be disallowed as is 
 practically always a bug, also, and it skips any initialisation 
 of the exception object. (See: http://dlang.org/statement.html 
 - "It is illegal for a GotoStatement to be used to skip 
 initializations.")

 Current failing test I want to have removed from the test suite.

 test/runnable/eh.d:
 void test8()
 {
   int a;
   goto L2;    // gdc Error: cannot goto into catch block

   try {
       a += 2;
   }
   catch (Exception e) {
       a += 3;
 L2: ;
       a += 100;
   }
   assert(a == 100);
 }


 Thanks
 Iain.


For what it's worth, this C++ program with GCC:

int main()
{
    goto L2;
    try {
        throw 5;
    }
    catch (int e) {
L2: ;
    }
}

produces:
main.cpp|| In function 'int main()':|
main.cpp|8|error: jump to label 'L2' [-fpermissive]|
main.cpp|3|error:   from here [-fpermissive]|
main.cpp|7|error:   crosses initialization of 'int e'|
main.cpp|8|error:   enters catch block|

I know you can add -permissive for GCC, or by default with VS*,
and crossed initliazers will be default constructed. That said, I
think it is mostly a workaround for legacy C, and *really* bad
practice to do it anyways.

So I think bottom line is: Invalid according to strict grammar,
and for a good reason.

*Amonst other things, like passing temporaries by ref. :puke: I
hate VS so much. PS: I'm responsible for cross compiling in my
company. The amount of shit VS allows is crazy scary.
Jun 13 2013
prev sibling next sibling parent reply Brad Roberts <braddr puremagic.com> writes:
On 6/13/13 10:35 AM, Iain Buclaw wrote:

 Can someone remind me again what was last agreed when I brought this up?

 I seem to recall that this should be disallowed as is practically always a
bug, also, and it skips
 any initialisation of the exception object. (See:
http://dlang.org/statement.html - "It is illegal
 for a GotoStatement to be used to skip initializations.")

 Current failing test I want to have removed from the test suite.

 test/runnable/eh.d:
 void test8()
 {
    int a;
    goto L2;    // gdc Error: cannot goto into catch block

    try {
        a += 2;
    }
    catch (Exception e) {
        a += 3;
 L2: ;
        a += 100;
    }
    assert(a == 100);
 }


 Thanks
 Iain.


I think it should be illegal, but not because it's a catch block but because of
the initialization. 
  If the catch was just "catch (Exception)" then it shouldn't be illegal.
Jun 13 2013
next sibling parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Thursday, 13 June 2013 at 18:51:01 UTC, Brad Roberts wrote:

 On 6/13/13 10:35 AM, Iain Buclaw wrote:

 I think it should be illegal, but not because it's a catch 
 block but because of the initialization.
  If the catch was just "catch (Exception)" then it shouldn't be 
 illegal.


Again for what it's worth, the C++ program with "catch(...)" also 
fails.

Even if the variable is unnamed though, doesn't the code still 
cross the declaration, which is enough to make a mess of things? 
I mean, the caught exception is still on the stack somewhere, 
right?

And still, if, by some hoops, we could make it work, do we really 
want to add that functionality? I mean, would it really even be 
useful?

That, and I'm not a fan to having code that breaks just because 
you later decided to name a variable.

Eg:
before:
catch(Exception) //OK
after
catch(Exception e) //Nope, that's illegal now. Good luck finding 
out why!
Jun 13 2013
next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Thursday, 13 June 2013 at 19:08:40 UTC, monarch_dodra wrote:

 On Thursday, 13 June 2013 at 18:51:01 UTC, Brad Roberts wrote:

 On 6/13/13 10:35 AM, Iain Buclaw wrote:

 I think it should be illegal, but not because it's a catch 
 block but because of the initialization.
 If the catch was just "catch (Exception)" then it shouldn't be 
 illegal.


 Again for what it's worth, the C++ program with "catch(...)" 
 also fails.


And for "catch(int)", this is interesting, it produces:
main.cpp||In function 'int main()':|
main.cpp|8|error: jump to label 'L2' [-fpermissive]|
main.cpp|3|error:   from here [-fpermissive]|
main.cpp|7|error:   crosses initialization of 'int <anonymous>'|
main.cpp|8|error:   enters catch block|

So yeah, just crossing a declaration, even anonymous, is enough 
for illegality in C++, hence simply no way to jump to inside a 
catch block.
Jun 13 2013
prev sibling parent Iain Buclaw <ibuclaw ubuntu.com> writes:
On 13 June 2013 20:08, monarch_dodra <monarchdodra gmail.com> wrote:

 On Thursday, 13 June 2013 at 18:51:01 UTC, Brad Roberts wrote:

 On 6/13/13 10:35 AM, Iain Buclaw wrote:

 I think it should be illegal, but not because it's a catch block but
 because of the initialization.
  If the catch was just "catch (Exception)" then it shouldn't be illegal.



 Again for what it's worth, the C++ program with "catch(...)" also fails.

 Even if the variable is unnamed though, doesn't the code still cross the
 declaration, which is enough to make a mess of things? I mean, the caught
 exception is still on the stack somewhere, right?

 And still, if, by some hoops, we could make it work, do we really want to
 add that functionality? I mean, would it really even be useful?

 That, and I'm not a fan to having code that breaks just because you later
 decided to name a variable.

 Eg:
 before:
 catch(Exception) //OK
 after
 catch(Exception e) //Nope, that's illegal now. Good luck finding out why!



Well, a good error message will do well in that case.

Currently, gdc has the following stock errors:

    "cannot goto into catch block"

Which is indeed confusing if anonymous catches are allowed to bypass
this check.  What I would instead do (once I have the scan for skipped
initialisations complete) is have a more meaningful, and generic:

    "goto to label 'L2' skips initialisation of 'Exception e'"


Ditto for any other forms of jumps over any other local variable declarations.


Regards
--
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
Jun 13 2013
prev sibling next sibling parent reply "Iain Buclaw" <ibuclaw ubuntu.com> writes:
On Thursday, 13 June 2013 at 18:51:01 UTC, Brad Roberts wrote:

 On 6/13/13 10:35 AM, Iain Buclaw wrote:

 Can someone remind me again what was last agreed when I 
 brought this up?

 I seem to recall that this should be disallowed as is 
 practically always a bug, also, and it skips
 any initialisation of the exception object. (See: 
 http://dlang.org/statement.html - "It is illegal
 for a GotoStatement to be used to skip initializations.")

 Current failing test I want to have removed from the test 
 suite.

 test/runnable/eh.d:
 void test8()
 {
   int a;
   goto L2;    // gdc Error: cannot goto into catch block

   try {
       a += 2;
   }
   catch (Exception e) {
       a += 3;
 L2: ;
       a += 100;
   }
   assert(a == 100);
 }


 Thanks
 Iain.


 I think it should be illegal, but not because it's a catch 
 block but because of the initialization.
  If the catch was just "catch (Exception)" then it shouldn't be 
 illegal.


This could be easily possible to do, and still keep 100% safe.  
But I must still ask why why why, Delilah, would you do that?
Jun 13 2013
next sibling parent reply Brad Roberts <braddr puremagic.com> writes:
On 6/13/13 12:22 PM, Iain Buclaw wrote:

 On Thursday, 13 June 2013 at 18:51:01 UTC, Brad Roberts wrote:

 On 6/13/13 10:35 AM, Iain Buclaw wrote:

 Can someone remind me again what was last agreed when I brought this up?

 I seem to recall that this should be disallowed as is practically always a
bug, also, and it skips
 any initialisation of the exception object. (See:
http://dlang.org/statement.html - "It is illegal
 for a GotoStatement to be used to skip initializations.")

 Current failing test I want to have removed from the test suite.

 test/runnable/eh.d:
 void test8()
 {
   int a;
   goto L2;    // gdc Error: cannot goto into catch block

   try {
       a += 2;
   }
   catch (Exception e) {
       a += 3;
 L2: ;
       a += 100;
   }
   assert(a == 100);
 }


 Thanks
 Iain.


 I think it should be illegal, but not because it's a catch block but because
of the initialization.
  If the catch was just "catch (Exception)" then it shouldn't be illegal.


 This could be easily possible to do, and still keep 100% safe. But I must
still ask why why why,
 Delilah, would you do that?


Oh, just minimizing what's illegal.  _I_ wouldn't do that.
Jun 13 2013
parent Walter Bright <newshound2 digitalmars.com> writes:
On 6/13/2013 3:53 PM, Brad Roberts wrote:

 Oh, just minimizing what's illegal.  _I_ wouldn't do that.


[Looks down, rubs toe in dirt.]
Jun 14 2013
prev sibling parent Iain Buclaw <ibuclaw ubuntu.com> writes:
On 13 June 2013 23:53, Brad Roberts <braddr puremagic.com> wrote:

 On 6/13/13 12:22 PM, Iain Buclaw wrote:

 On Thursday, 13 June 2013 at 18:51:01 UTC, Brad Roberts wrote:

 On 6/13/13 10:35 AM, Iain Buclaw wrote:

 Can someone remind me again what was last agreed when I brought this up?

 I seem to recall that this should be disallowed as is practically always
 a bug, also, and it skips
 any initialisation of the exception object. (See:
 http://dlang.org/statement.html - "It is illegal
 for a GotoStatement to be used to skip initializations.")

 Current failing test I want to have removed from the test suite.

 test/runnable/eh.d:
 void test8()
 {
   int a;
   goto L2;    // gdc Error: cannot goto into catch block

   try {
       a += 2;
   }
   catch (Exception e) {
       a += 3;
 L2: ;
       a += 100;
   }
   assert(a == 100);
 }


 Thanks
 Iain.



 I think it should be illegal, but not because it's a catch block but
 because of the initialization.
  If the catch was just "catch (Exception)" then it shouldn't be illegal.



 This could be easily possible to do, and still keep 100% safe. But I must
 still ask why why why,
 Delilah, would you do that?



 Oh, just minimizing what's illegal.  _I_ wouldn't do that.


OK, and just out of curiosity, what's your view on jumps into try
statement blocks?

goto L2;

try {
  /* ... */
L2:
  /* ... */
}


Obviously the same rule must apply if the jump to the label skips over
any initialisations at the beginning of the try block above it...

--
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
Jun 13 2013
prev sibling parent Timon Gehr <timon.gehr gmx.ch> writes:
On 06/13/2013 08:50 PM, Brad Roberts wrote:

 On 6/13/13 10:35 AM, Iain Buclaw wrote:

 Can someone remind me again what was last agreed when I brought this up?

 I seem to recall that this should be disallowed as is practically
 always a bug, also, and it skips
 any initialisation of the exception object. (See:
 http://dlang.org/statement.html - "It is illegal
 for a GotoStatement to be used to skip initializations.")

 Current failing test I want to have removed from the test suite.

 test/runnable/eh.d:
 void test8()
 {
    int a;
    goto L2;    // gdc Error: cannot goto into catch block

    try {
        a += 2;
    }
    catch (Exception e) {
        a += 3;
 L2: ;
        a += 100;
    }
    assert(a == 100);
 }


 Thanks
 Iain.


 I think it should be illegal, but not because it's a catch block but
 because of the initialization.  If the catch was just "catch
 (Exception)" then it shouldn't be illegal.


+1.
Jun 13 2013
prev sibling next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Thu, Jun 13, 2013 at 11:50:49AM -0700, Brad Roberts wrote:

 On 6/13/13 10:35 AM, Iain Buclaw wrote:

Can someone remind me again what was last agreed when I brought this
up?

I seem to recall that this should be disallowed as is practically
always a bug, also, and it skips any initialisation of the exception
object. (See: http://dlang.org/statement.html - "It is illegal for a
GotoStatement to be used to skip initializations.")

Current failing test I want to have removed from the test suite.

test/runnable/eh.d:
void test8()
{
   int a;
   goto L2;    // gdc Error: cannot goto into catch block

   try {
       a += 2;
   }
   catch (Exception e) {
       a += 3;
L2: ;
       a += 100;
   }
   assert(a == 100);
}


Thanks
Iain.


 
 I think it should be illegal, but not because it's a catch block but
 because of the initialization.  If the catch was just "catch
 (Exception)" then it shouldn't be illegal.


Why shouldn't it be illegal, though? I honestly don't see any use case
for such a strange construct. Not to mention, like monarch_dodra said,
if the anonymous Exception's reference is on the stack, then at the end
of the catch block there'd be code to adjust the stack pointer, which
will trash your stack pointer horribly if we goto the middle of the
block bypassing the stack allocation of the Exception reference.


T

-- 
What's a "hot crossed bun"? An angry rabbit.
Jun 13 2013
prev sibling parent reply Iain Buclaw <ibuclaw ubuntu.com> writes:
On 13 June 2013 20:18, H. S. Teoh <hsteoh quickfur.ath.cx> wrote:

 On Thu, Jun 13, 2013 at 11:50:49AM -0700, Brad Roberts wrote:

 I think it should be illegal, but not because it's a catch block but
 because of the initialization.  If the catch was just "catch
 (Exception)" then it shouldn't be illegal.


 Why shouldn't it be illegal, though? I honestly don't see any use case
 for such a strange construct. Not to mention, like monarch_dodra said,
 if the anonymous Exception's reference is on the stack, then at the end
 of the catch block there'd be code to adjust the stack pointer, which
 will trash your stack pointer horribly if we goto the middle of the
 block bypassing the stack allocation of the Exception reference.


Thank goodness D's exceptions are all heap allocated!

--
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
Jun 13 2013
parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Thursday, 13 June 2013 at 20:40:27 UTC, Iain Buclaw wrote:

 On 13 June 2013 20:18, H. S. Teoh <hsteoh quickfur.ath.cx> 
 wrote:

 On Thu, Jun 13, 2013 at 11:50:49AM -0700, Brad Roberts wrote:

 I think it should be illegal, but not because it's a catch 
 block but
 because of the initialization.  If the catch was just "catch
 (Exception)" then it shouldn't be illegal.


 Why shouldn't it be illegal, though? I honestly don't see any 
 use case
 for such a strange construct. Not to mention, like 
 monarch_dodra said,
 if the anonymous Exception's reference is on the stack, then 
 at the end
 of the catch block there'd be code to adjust the stack 
 pointer, which
 will trash your stack pointer horribly if we goto the middle 
 of the
 block bypassing the stack allocation of the Exception 
 reference.


 Thank goodness D's exceptions are all heap allocated!

 --
 Iain Buclaw

 *(p < e ? p++ : p) = (c & 0x0f) + '0';


Irrelevant no? You're still throwing around your pointer, which 
takes its stack space.

Well, I guess as long as the compiler (optionally) supports 
skipping over initialization it means it can handle the magic 
required to maintain the stack pointer, so that shouldn't really 
be a problem.
Jun 13 2013
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 6/13/2013 2:08 PM, monarch_dodra wrote:

 Well, I guess as long as the compiler (optionally) supports skipping over
 initialization it means it can handle the magic required to maintain the stack
 pointer, so that shouldn't really be a problem.


Sometimes it is handy to skip initializations, and I get annoyed with g++ barfs 
on me doing that. That said, such should only be allowed in  system code.

On a related note, doing a goto into a finally block is pretty problematic to 
support because it's actually a mini-function.
Jun 14 2013
next sibling parent Iain Buclaw <ibuclaw ubuntu.com> writes:
On 14 June 2013 19:44, Walter Bright <newshound2 digitalmars.com> wrote:

 On 6/13/2013 2:08 PM, monarch_dodra wrote:

 Well, I guess as long as the compiler (optionally) supports skipping over
 initialization it means it can handle the magic required to maintain the
 stack
 pointer, so that shouldn't really be a problem.



 Sometimes it is handy to skip initializations, and I get annoyed with g++
 barfs on me doing that. That said, such should only be allowed in  system
 code.


IMO, the only annoying ones in C++ are where switch statements jumping
to case labels crosses initialisation of variables, but D solves that
by having cases in their own scope. :o)

Any other type of skipped initialisation is an error that should
definitely have all alarm bells ringing.

--
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
Jun 14 2013
prev sibling parent Timon Gehr <timon.gehr gmx.ch> writes:
On 06/14/2013 08:44 PM, Walter Bright wrote:

 On 6/13/2013 2:08 PM, monarch_dodra wrote:

 Well, I guess as long as the compiler (optionally) supports skipping over
 initialization it means it can handle the magic required to maintain
 the stack
 pointer, so that shouldn't really be a problem.


 Sometimes it is handy to skip initializations, and I get annoyed with
 g++ barfs on me doing that. That said, such should only be allowed in
  system code.

 On a related note, doing a goto into a finally block is pretty
 problematic to support because it's actually a mini-function.


In the worst case, duplicate some code.
Jun 14 2013