Bug 227: Fix spurious warning of function returning address of local variable.

[ibuclaw]

• First, this does a small refactor/clean-up of codegen for passing parameters that have been lowered to a COMPOUND_EXPR, spliting the value from the rest of the construction/destruction expression if possible.
• A similar rewrite is done in build_expr, where the following rewrites are done for simple expressions:

  cast(T)(e1, e2) => (e1, cast(T)e2)
  &(e1, e2) => (e1, &e2)
  *(e1, e2) => (e1, *e2)

Moving the COMPOUND_EXPR up to the top allows us to have more control on when inserting dtors into the expression.

Where before we may have done (a contrived example):

  SAVE<tmp = __ctor(), tmp>, __dtor(&tmp), SAVE<tmp = __ctor(), tmp>;

Rewriting &(e1, e2) into (e1, &e2) means we can inspect the right hand side for any side effects, if there are none pressent, then we can bypass saving the comma expression, eliding any artificial temporaries being created by the code generator/optimizer.

  tmp = __ctor(), __dtor(&tmp), tmp;

• Finally, for fixing bug 227, the code generation for running destructors on return expressions has been separated from normal expressions, because we should be returning the result directly, rather than returning the temporary created to evaluate the expression before calling its destructors.

  return &(try
    {
      SAVE<*(SAVE<*(SAVE<&(tmp = __ctor(), tmp)>), {}>,
           __cpctor(this, SAVE<*(SAVE<&(tmp = __ctor(), tmp)>), {}>))>;
    }
  finally
    {
      __dtor(&tmp);
    }, SAVE<*(SAVE<*(SAVE<&(tmp = __ctor(), tmp)>), {}>,
            __cpctor(this, SAVE<*(SAVE<&(tmp = __ctor(), tmp)>), {}>))>);

With the first refactor in passing parameters, we remove one layer of temporaries being created in the call to __cpctor().

  return &(try
    {
      SAVE<*(*(SAVE<&(tmp = __ctor(), tmp)>), __cpctor(this, {}))>;
    }
  finally
    {
      __dtor(&tmp);
    }, SAVE<*(*(SAVE<&(tmp = __ctor(), tmp)>), __cpctor(this, {}))>);

With the second refactor, another layer is removed, as the construction of the temporary is not needed when dereferencing the result of __cpctor().

  return &(try
    {
      *(SAVE<&(tmp = __ctor(), tmp)>);, SAVE<*__cpctor(this, {})>;;
    }
  finally
    {
      __dtor(&tmp);
    }, SAVE<*__cpctor(this, {})>;);

Finally, moving the return expression into build_return_dtor, the return is inserted directory into the try block, removing the bogus temporary that was causing the warning in the first place.

  try
    {
      *(SAVE<&(tmp = __ctor(), tmp)>);
      return __cpctor(this, {});
    }
  finally
    {
      __dtor(&tmp);
    }

[ibuclaw]

Should probably look into writing a pass that un-saves expressions that are not evaluated more than once. But so far this small improvement is a far cry better than what we started with.

@jpf91 - Any problems you can think of as a result of this?

[jpf91]

@jpf91 - Any problems you can think of as a result of this?

No. Sounds like a good idea 👍

[ibuclaw]

OK - let's merge this first as it has no caught regressions. :-)

[jpf91]

@ibuclaw The changes in build_expr_dtor caused a regression in gdc-4.8: https://semaphoreci.com/jpf91/gdc/branches/gdc-4-8/builds/40

Consider this code:

void assertThrown(T : Throwable = Exception, E)
                 (lazy E expression,
                  string msg = null,
                  string file = __FILE__,
                  size_t line = __LINE__)
{
    import core.exception : AssertError;

    try
        expression();
    catch (T)
        return;
    throw new AssertError("assertThrown failed: No " ~ T.stringof ~ " was thrown"
                                 ~ (msg.length == 0 ? "." : ": ") ~ msg,
                          file, line);
}

(complete example: https://paste.gnome.org/pshloltok compile: gdc -funittest)

Without this pull request:

;; Function assertThrown (_D7process51__T12assertThrownHTC6object5ErrorTS3std5stdio4FileZ12assertThrownFLS3std5stdio4FileAyaAyamZv)
;; enabled by -tree-original

{
  try
    {
      {
        struct File __tmpfordtor2916;

        __tmpfordtor2916 = VIEW_CONVERT_EXPR<struct File delegate() pure @safe>(expression).func (VIEW_CONVERT_EXPR<struct File delegate() pure @safe>(expression).object) [return slot optimization];, __dtor (&__tmpfordtor2916);;, __tmpfordtor2916;
      }
    }
  catch
    {
      catch (struct Error *)
        {
          __gdc_begin_catch (__builtin_eh_pointer (0));
          return;
        }
    }
  _d_throw (SAVE_EXPR <(struct AssertError *) _d_newclass (&core.exception.AssertError.__Class)>;, SAVE_EXPR <{
    struct byte[] D.9380[3];

    D.9380 = {[2]=msg, [1]=msg.length == 0 ? {.length=1, .ptr="."} : {.length=2, .ptr=": "}, [0]={.length=40, .ptr="assertThrown failed: No Error was thrown"}};, VIEW_CONVERT_EXPR<struct string>(_d_arraycatnTX (&_D12TypeInfo_Aya6__initZ, {.length=3, .ptr=(struct byte[] *) &D.9380}));;
  }>;;, (struct Object *) __ctor (SAVE_EXPR <(struct AssertError *) _d_newclass (&core.exception.AssertError.__Class)>, SAVE_EXPR <{
    struct byte[] D.9380[3];

    D.9380 = {[2]=msg, [1]=msg.length == 0 ? {.length=1, .ptr="."} : {.length=2, .ptr=": "}, [0]={.length=40, .ptr="assertThrown failed: No Error was thrown"}};, VIEW_CONVERT_EXPR<struct string>(_d_arraycatnTX (&_D12TypeInfo_Aya6__initZ, {.length=3, .ptr=(struct byte[] *) &D.9380}));;
  }>, file, line, 0B););
}

With this pull request:

;; Function assertThrown (_D7process51__T12assertThrownHTC6object5ErrorTS3std5stdio4FileZ12assertThrownFLS3std5stdio4FileAyaAyamZv)
;; enabled by -tree-original

{
  try
    {
      {
        struct File __tmpfordtor2916;

        try
          {
            __tmpfordtor2916 = VIEW_CONVERT_EXPR<struct File delegate() pure @safe>(expression).func (VIEW_CONVERT_EXPR<struct File delegate() pure @safe>(expression).object) [return slot optimization];, __tmpfordtor2916;
          }
        finally
          {
            __dtor (&__tmpfordtor2916);
          }, __tmpfordtor2916;
      }
    }
  catch
    {
      catch (struct Error *)
        {
          __gdc_begin_catch (__builtin_eh_pointer (0));
          return;
        }
    }
  _d_throw (SAVE_EXPR <(struct AssertError *) _d_newclass (&core.exception.AssertError.__Class)>;, SAVE_EXPR <{
    struct byte[] D.9380[3];

    D.9380 = {[2]=msg, [1]=msg.length == 0 ? {.length=1, .ptr="."} : {.length=2, .ptr=": "}, [0]={.length=40, .ptr="assertThrown failed: No Error was thrown"}};, VIEW_CONVERT_EXPR<struct string>(_d_arraycatnTX (&_D12TypeInfo_Aya6__initZ, {.length=3, .ptr=(struct byte[] *) &D.9380}));;
  }>;;, (struct Object *) __ctor (SAVE_EXPR <(struct AssertError *) _d_newclass (&core.exception.AssertError.__Class)>, SAVE_EXPR <{
    struct byte[] D.9380[3];

    D.9380 = {[2]=msg, [1]=msg.length == 0 ? {.length=1, .ptr="."} : {.length=2, .ptr=": "}, [0]={.length=40, .ptr="assertThrown failed: No Error was thrown"}};, VIEW_CONVERT_EXPR<struct string>(_d_arraycatnTX (&_D12TypeInfo_Aya6__initZ, {.length=3, .ptr=(struct byte[] *) &D.9380}));;
  }>, file, line, 0B););
}

The additional try/finally means we now call the dtor even if expression() threw an Exception. Was this intentional? I don't know the exact language rules, but it seems to be wrong: As __tmpfordtor2916 is only initialized in the expression call and if that call fails it's not initialized at all => we shouldn't call the destructor?

(The gcc-4.8 regression is actually caused by the destructor getting uninitialized data. File checks for if(_p==null) in the destructor, so it does not cause problems for default initialized fields. For some reason in gcc 4.8 the temporary is not zero initialized but it is for all newer GCCs. I haven't looked into this in detail, might be a unrelated gcc-4.8 bug or maybe it's just a valid difference in the codegen.)

[ibuclaw]

I guess it is here that triggered the change. Before, only TOKcall and TOKnew expressions were wrapped in a try/finally. Now it is all expressions except constructors.

I considered this reasonable, but maybe an exception could be made for temporaries as well. I'll have to think on it.