Re: comctl32[2/2]: toolbar: fix the layout of TBUTTON_INFO on Win64

[Dmitry Timoshkov]

"Mikolaj Zalewski"  wrote:
> +/* Note: TOOLBAR_DumpButton assumes the layout of the beginning of the structure
> -
> - is the same as of TBBUTTON */
>
> typedef struct
> {
>     INT iBitmap;
> @@ -96,6 +98,9 @@ typedef struct
>     BYTE  fsStyle;
>     BYTE  bHot;
>     BYTE  bDropDownPressed;
> +#ifdef _WIN64
> - BYTE bReserved64[4];
>
> +#endif
>     DWORD_PTR dwData;
>     INT_PTR iString;
>     INT nRow;

Then probably it would make sense to make TBBUTTON the first member of the above
structure. That would help in avoiding possible layout problems in future.

comctl32[2/2]: toolbar: fix the layout of TBUTTON_INFO on Win64

[Mikołaj Zalewski]

Dmitry Timoshkov wrote:
> "Mikolaj Zalewski"  wrote:
> > +/* Note: TOOLBAR_DumpButton assumes the layout of the beginning of 
> > the structure
> > -
> > - is the same as of TBBUTTON */
> >
> > typedef struct
> > {
> >     INT iBitmap;
> > @@ -96,6 +98,9 @@ typedef struct
> >     BYTE  fsStyle;
> >     BYTE  bHot;
> >     BYTE  bDropDownPressed;
> > +#ifdef _WIN64
> > - BYTE bReserved64[4];
> >
> > +#endif
> >     DWORD_PTR dwData;
> >     INT_PTR iString;
> >     INT nRow;
>
> Then probably it would make sense to make TBBUTTON the first member of 
> the above
> structure. That would help in avoiding possible layout problems in 
> future.

The bHot and bDropDownPressed would then be called bReserved[0] and 
bReserved[1] so this could make more harm than good.

Mikolaj

comctl32[2/2]: toolbar: fix the layout of TBUTTON_INFO on Win64

[Dmitry Timoshkov]

"Mikołaj Zalewski"  wrote:
> > > +/* Note: TOOLBAR_DumpButton assumes the layout of the beginning of the structure
> > > -
> > > - is the same as of TBBUTTON */
> > >
> > > typedef struct
> > > {
> > >     INT iBitmap;
> > > @@ -96,6 +98,9 @@ typedef struct
> > >     BYTE  fsStyle;
> > >     BYTE  bHot;
> > >     BYTE  bDropDownPressed;
> > > +#ifdef _WIN64
> > > - BYTE bReserved64[4];
> > >
> > > +#endif
> > >     DWORD_PTR dwData;
> > >     INT_PTR iString;
> > >     INT nRow;
> >
> > Then probably it would make sense to make TBBUTTON the first member of the above
> > structure. That would help in avoiding possible layout problems in future.
>
> The bHot and bDropDownPressed would then be called bReserved[0] and bReserved[1] so this could make more harm than 
> good.

You can make a union for that case, with an appropriate comment.

comctl32[2/2]: toolbar: fix the layout of TBUTTON_INFO on Win64

[Mikołaj Zalewski]

Dmitry Timoshkov wrote:
> "Mikołaj Zalewski"  wrote:
> > > > +/* Note: TOOLBAR_DumpButton assumes the layout of the beginning of 
> > > > the structure
> > > > -
> > > > - is the same as of TBBUTTON */
> > > >
> > > > typedef struct
> > > > {
> > > >     INT iBitmap;
> > > > @@ -96,6 +98,9 @@ typedef struct
> > > >     BYTE  fsStyle;
> > > >     BYTE  bHot;
> > > >     BYTE  bDropDownPressed;
> > > > +#ifdef _WIN64
> > > > - BYTE bReserved64[4];
> > > >
> > > > +#endif
> > > >     DWORD_PTR dwData;
> > > >     INT_PTR iString;
> > > >     INT nRow;
> > >
> > > Then probably it would make sense to make TBBUTTON the first member 
> > > of the above
> > > structure. That would help in avoiding possible layout problems in 
> > > future.
> >
> > The bHot and bDropDownPressed would then be called bReserved[0] and 
> > bReserved[1] so this could make more harm than good.
>
> You can make a union for that case, with an appropriate comment.

TBBUTTON is an official Windows API structure. Adding a union inside 
and doing it just because of some internals of our toolbar 
implementation is IMHO not the best idea.

Mikolaj

comctl32[2/2]: toolbar: fix the layout of TBUTTON_INFO on Win64

[Alexandre Julliard]

Mikołaj Zalewski  writes:
> Dmitry Timoshkov wrote:
> > "Mikołaj Zalewski"  wrote:
> > > The bHot and bDropDownPressed would then be called bReserved[0] and 
> > > bReserved[1] so this could make more harm than good.
> >
> > You can make a union for that case, with an appropriate comment.
>
> TBBUTTON is an official Windows API structure. Adding a union inside 
> and doing it just because of some internals of our toolbar 
> implementation is IMHO not the best idea.

If there is evidence that Windows really uses the reserved fields to
store bHot and bDropDownPressed then the code should be using
bReserved[0] and bReserved[1], wrapped with appropriate accessor
functions if necessary. Otherwise they should be moved somewhere else
and the bReserved fields left alone. Either way, if we need the correct
TBBUTTON layout we should be using a TBBUTTON structure.

comctl32[2/2]: toolbar: fix the layout of TBUTTON_INFO on Win64

[Mikołaj Zalewski]

Alexandre Julliard wrote:
> If there is evidence that Windows really uses the reserved fields to
> store bHot and bDropDownPressed then the code should be using
> bReserved[0] and bReserved[1], wrapped with appropriate accessor
> functions if necessary. Otherwise they should be moved somewhere else
> and the bReserved fields left alone. Either way, if we need the correct
> TBBUTTON layout we should be using a TBBUTTON structure.

The bHot and bDropDownPressed fields are in the TBUTTON_INFO structure 
that is internal to our implementation. It is not visible to the outside 
world. Probably for convinience, one debug function (TOOLBAR_DumpButton) 
assumes that the first fields of it are the same as of TBBUTTON and the 
same function is used dump  TBBUTTON and TBUTTON_INFO (with a flag to 
dump the additional fields). When I noticed this won't work on Win64, I 
thought adding the padding is the easiest solution. Another solution 
would be to split the dump function into one for TBUTTON_INFO and one 
TBBUTTON.

Mikołaj