Re: [v2 02/10] msvcirt: Implement ostream constructors/destructors

[Piotr Caban]

On 06/14/16 19:01, Iván Matellanes wrote:
> On 14/06/16 16:20, Piotr Caban wrote:
> > On 06/14/16 16:40, Iván Matellanes wrote:
> > > /* ??_Dostream@@QAEXXZ */
> > > @@ -2287,23 +2341,48 @@ ostream* __thiscall ostream_assign(ostream
> > > *this, const ostream *rhs)
> > >  DEFINE_THISCALL_WRAPPER(ostream_vbase_dtor, 4)
> > >  void __thiscall ostream_vbase_dtor(ostream *this)
> > >  {
> > > - FIXME("(%p) stub\n", this);
> > > - ios *base = ostream_to_ios(this);
> > > -
> > > - TRACE("(%p)\n", this);
> > > -
> > > - ostream_dtor(base);
> > > - ios_dtor(base);
> > >
> > > }
> >
> > The base should be computed using ostream_get_ios while calling ios_dtor:
> > TRACE("(%p)\n", this);
> > ostream_dtor(ostream_to_ios(this));
> > ios_dtor(ostream_get_ios(this));
>
> Do you mind explaining why is that required? I can't see the reason

ostream_vbase_dtor can be used to destroy object that has virtual base 
in any position pointer by vbtable. ostream_get_ios computes it using 
object's vbtable. You can easily see it by overwriting ostream->vbtable 
with garbage, the application will crash then on Windows.

On the other hand ostream_dtor is a virtual function that assumes "this" 
is at fixed offset from "base" that is passed to it. It's done this way 
because it has no access to vbtable. It's why you need to use 
ostream_to_ios in this case. There's no real need to call ostream_dtor 
here because it's an empty function (I don't know if it should be called 
here or if ostream_vbase_dtor should only destroy the vbase).

Thanks,
Piotr

[v2 02/10] msvcirt: Implement ostream constructors/destructors

[Iván Matellanes]

On 14/06/16 19:59, Piotr Caban wrote:
> On 06/14/16 19:01, Iván Matellanes wrote:
> > On 14/06/16 16:20, Piotr Caban wrote:
> > > On 06/14/16 16:40, Iván Matellanes wrote:
> > > > /* ??_Dostream@@QAEXXZ */
> > > > @@ -2287,23 +2341,48 @@ ostream* __thiscall ostream_assign(ostream
> > > > *this, const ostream *rhs)
> > > >  DEFINE_THISCALL_WRAPPER(ostream_vbase_dtor, 4)
> > > >  void __thiscall ostream_vbase_dtor(ostream *this)
> > > >  {
> > > > - FIXME("(%p) stub\n", this);
> > > > - ios *base = ostream_to_ios(this);
> > > > -
> > > > - TRACE("(%p)\n", this);
> > > > -
> > > > - ostream_dtor(base);
> > > > - ios_dtor(base);
> > > >
> > > > }
> > >
> > > The base should be computed using ostream_get_ios while calling 
> > > ios_dtor:
> > > TRACE("(%p)\n", this);
> > > ostream_dtor(ostream_to_ios(this));
> > > ios_dtor(ostream_get_ios(this));
> >
> > Do you mind explaining why is that required? I can't see the reason
>
> ostream_vbase_dtor can be used to destroy object that has virtual base 
> in any position pointer by vbtable. ostream_get_ios computes it using 
> object's vbtable. You can easily see it by overwriting 
> ostream->vbtable with garbage, the application will crash then on Windows.

It crashes when ostream->vbtable is garbage, but otherwise, it seems to 
ignore the value and destroy the ios object at a fixed position anyway 
(see attached file). I think this is reasonable since ostream_vbase_dtor 
will only be used to destroy ostream objects, i.e., not objects of 
subclasses, right?
This doesn't explain why we get a crash when ostream->vbtable = NULL though.
> On the other hand ostream_dtor is a virtual function that assumes 
> "this" is at fixed offset from "base" that is passed to it. It's done 
> this way because it has no access to vbtable. It's why you need to use 
> ostream_to_ios in this case. There's no real need to call ostream_dtor 
> here because it's an empty function (I don't know if it should be 
> called here or if ostream_vbase_dtor should only destroy the vbase).

True, the function call is a no-op. I was following the style in msvcp90 
to be coherent, but I can remove this call if you feel it makes more sense.

Cheers,
Iván

[v2 02/10] msvcirt: Implement ostream constructors/destructors

[Piotr Caban]

On 06/14/16 23:42, Iván Matellanes wrote:
> It crashes when ostream->vbtable is garbage, but otherwise, it seems to
> ignore the value and destroy the ios object at a fixed position anyway
> (see attached file). I think this is reasonable since ostream_vbase_dtor
> will only be used to destroy ostream objects, i.e., not objects of
> subclasses, right?

Yes, you're right. I've done some more testing and it turns out `vbase 
destructors` are not using vbtable.
> This doesn't explain why we get a crash when ostream->vbtable = NULL
> though.

The crash is caused by destructor. It turns out the destructor is always 
updating virtual functions table. The vbtable is used to compute virtual 
functions table location. E.g.
class a destructor does something like this:
void thiscall a_dtor(base *b) {
     a *this = base_to_a(base);
     b = a_get_base(a);
     b->vtable = &MSVCP_a_vtable;
     //do the object destruction ...
}
The virtual function table is updated in all destructors (even if 
virtual inheritance is not used). As long as we're not calling virtual 
functions in destructors there's probably no need to do it in wine.

Thanks,
Piotr