[PATCH] msvcp90: Fix vtable alignment on macOS.

[Ken Thomases]

On macOS, the .align <n> directive aligns to 2^n, not just n.

Signed-off-by: Ken Thomases 
---
 dlls/msvcp90/cxx.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/dlls/msvcp90/cxx.h b/dlls/msvcp90/cxx.h
index 917d814..86295cd 100644
--- a/dlls/msvcp90/cxx.h
+++ b/dlls/msvcp90/cxx.h
@@ -43,9 +43,15 @@

#define VTABLE_ADD_FUNC(name) "\t.quad " THISCALL_NAME(name) "\n"

+#ifdef __APPLE__
+#define __ASM_VTABLE_ALIGN ".align 3"
+#else
+#define __ASM_VTABLE_ALIGN ".align 8"
+#endif
+
 #define __ASM_VTABLE(name,funcs) \
     __asm__(".data\n" \
-            "\t.align 8\n" \
+            "\t" __ASM_VTABLE_ALIGN "\n" \
             "\t.quad " __ASM_NAME(#name "_rtti") "\n" \
             "\t.globl " __ASM_NAME("MSVCP_" #name "_vtable") "\n" \
             __ASM_NAME("MSVCP_" #name "_vtable") ":\n" \
@@ -55,9 +61,15 @@

#define VTABLE_ADD_FUNC(name) "\t.long " THISCALL_NAME(name) "\n"

+#ifdef __APPLE__
+#define __ASM_VTABLE_ALIGN ".align 2"
+#else
+#define __ASM_VTABLE_ALIGN ".align 4"
+#endif
+
 #define __ASM_VTABLE(name,funcs) \
     __asm__(".data\n" \
-            "\t.align 4\n" \
+            "\t" __ASM_VTABLE_ALIGN "\n" \
             "\t.long " __ASM_NAME(#name "_rtti") "\n" \
             "\t.globl " __ASM_NAME("MSVCP_" #name "_vtable") "\n" \
             __ASM_NAME("MSVCP_" #name "_vtable") ":\n" \

[Chip Davis]

March 6, 2019 7:01 PM, "Ken Thomases"  wrote:
> On macOS, the .align <n> directive aligns to 2^n, not just n.

I wonder if we should explicitly use '.balign 8', since that always means "align to 8 bytes." Unless, of course, there are platforms that don't support it.
> Signed-off-by: Ken Thomases
>
> dlls/msvcp90/cxx.h | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/msvcp90/cxx.h b/dlls/msvcp90/cxx.h
> index 917d814..86295cd 100644
> --- a/dlls/msvcp90/cxx.h
> +++ b/dlls/msvcp90/cxx.h
> @@ -43,9 +43,15 @@
>
> #define VTABLE_ADD_FUNC(name) "\t.quad " THISCALL_NAME(name) "\n"
>
> +#ifdef __APPLE__
> +#define __ASM_VTABLE_ALIGN ".align 3"
> +#else
> +#define __ASM_VTABLE_ALIGN ".align 8"
> +#endif
> -
>
> #define __ASM_VTABLE(name,funcs) \
> __asm__(".data\n" \
> - "\t.align 8\n" \
> - "\t" __ASM_VTABLE_ALIGN "\n" \
>
> "\t.quad " __ASM_NAME(#name "_rtti") "\n" \
> "\t.globl " __ASM_NAME("MSVCP_" #name "_vtable") "\n" \
> __ASM_NAME("MSVCP_" #name "_vtable") ":\n" \
> @@ -55,9 +61,15 @@
>
> #define VTABLE_ADD_FUNC(name) "\t.long " THISCALL_NAME(name) "\n"
>
> +#ifdef __APPLE__
> +#define __ASM_VTABLE_ALIGN ".align 2"
> +#else
> +#define __ASM_VTABLE_ALIGN ".align 4"
> +#endif
> -
>
> #define __ASM_VTABLE(name,funcs) \
> __asm__(".data\n" \
> - "\t.align 4\n" \
> - "\t" __ASM_VTABLE_ALIGN "\n" \
>
> "\t.long " __ASM_NAME(#name "_rtti") "\n" \
> "\t.globl " __ASM_NAME("MSVCP_" #name "_vtable") "\n" \
> __ASM_NAME("MSVCP_" #name "_vtable") ":\n" \
> -- 
> 2.10.2

Chip

[Ken Thomases]

On Mar 6, 2019, at 9:28 PM, Chip Davis  wrote:
> March 6, 2019 7:01 PM, "Ken Thomases"  wrote:
> > On macOS, the .align <n> directive aligns to 2^n, not just n.
>
> I wonder if we should explicitly use '.balign 8', since that always means "align to 8 bytes." Unless, of course, there are platforms that don't support it.

This is a general issue that winebuild also explicitly contends with, and it isn't using .balign (nor .p2align).  See tools/winebuild/utils.c:get_alignment().  So, I assume .balign isn't an acceptable solution.  I'd be happy to be told otherwise, though.

-Ken

[Alexandre Julliard]

Ken Thomases  writes:
> On Mar 6, 2019, at 9:28 PM, Chip Davis  wrote:
> > March 6, 2019 7:01 PM, "Ken Thomases"  wrote:
> > > On macOS, the .align <n> directive aligns to 2^n, not just n.
> >
> > I wonder if we should explicitly use '.balign 8', since that always means "align to 8 bytes." Unless, of course, there are platforms that don't support it.
>
> This is a general issue that winebuild also explicitly contends with,
> and it isn't using .balign (nor .p2align).  See
> tools/winebuild/utils.c:get_alignment().  So, I assume .balign isn't
> an acceptable solution.  I'd be happy to be told otherwise, though.

It's not supported on Solaris, which was the reason for the winebuild
code. I'm not sure if anybody still cares about Solaris nowadays...

[Piotr Caban]

Hi Ken,

On 3/7/19 9:47 AM, Alexandre Julliard wrote:
> Ken Thomases  writes:
> > On Mar 6, 2019, at 9:28 PM, Chip Davis  wrote:
> > > March 6, 2019 7:01 PM, "Ken Thomases"  wrote:
> > > > On macOS, the .align <n> directive aligns to 2^n, not just n.
> > >
> > > I wonder if we should explicitly use '.balign 8', since that always means "align to 8 bytes." Unless, of course, there are platforms that don't support it.
> >
> > This is a general issue that winebuild also explicitly contends with,
> > and it isn't using .balign (nor .p2align).  See
> > tools/winebuild/utils.c:get_alignment().  So, I assume .balign isn't
> > an acceptable solution.  I'd be happy to be told otherwise, though.
>
> It's not supported on Solaris, which was the reason for the winebuild
> code. I'm not sure if anybody still cares about Solaris nowadays...

Shouldn't it be changed on ARM, ARM64 and PowerPC as well? Maybe it's 
better to use .balign for all platforms except Solaris (or ignore 
Solaris until someone reports bug for it?).

Thanks,
Piotr

[Michael Stefaniuc]

On 3/7/19 9:47 AM, Alexandre Julliard wrote:
> Ken Thomases  writes:
> > On Mar 6, 2019, at 9:28 PM, Chip Davis  wrote:
> > > March 6, 2019 7:01 PM, "Ken Thomases"  wrote:
> > > > On macOS, the .align <n> directive aligns to 2^n, not just n.
> > >
> > > I wonder if we should explicitly use '.balign 8', since that always means "align to 8 bytes." Unless, of course, there are platforms that don't support it.
> >
> > This is a general issue that winebuild also explicitly contends with,
> > and it isn't using .balign (nor .p2align).  See
> > tools/winebuild/utils.c:get_alignment().  So, I assume .balign isn't
> > an acceptable solution.  I'd be happy to be told otherwise, though.
>
> It's not supported on Solaris, which was the reason for the winebuild
> code. I'm not sure if anybody still cares about Solaris nowadays..

The last commit mentioning Solaris is from 2016 Francois.
So if he doesn't cares anymore about it I doubt anybody else does.

bye
    michael

[Austin English]

On Thu, Mar 7, 2019, 09:48 Alexandre Julliard  wrote:
> Ken Thomases  writes:
> > On Mar 6, 2019, at 9:28 PM, Chip Davis  wrote:
> > > March 6, 2019 7:01 PM, "Ken Thomases"  wrote:
> > > > On macOS, the .align <n> directive aligns to 2^n, not just n.
> > >
> > > I wonder if we should explicitly use '.balign 8', since that always
>
> means "align to 8 bytes." Unless, of course, there are platforms that don't
> support it.
> > This is a general issue that winebuild also explicitly contends with,
> > and it isn't using .balign (nor .p2align).  See
> > tools/winebuild/utils.c:get_alignment().  So, I assume .balign isn't
> > an acceptable solution.  I'd be happy to be told otherwise, though.
>
> It's not supported on Solaris, which was the reason for the winebuild
> code. I'm not sure if anybody still cares about Solaris nowadays...
>
> --
> Alexandre Julliard
> [e-mail]

Does this also mean we can kill backticks in shell scripts? ;)
>

[Alexandre Julliard]

Austin English  writes:
> On Thu, Mar 7, 2019, 09:48 Alexandre Julliard  wrote:
>
> Ken Thomases  writes:
> > On Mar 6, 2019, at 9:28 PM, Chip Davis  wrote:
> > > March 6, 2019 7:01 PM, "Ken Thomases"  wrote:
> > > > On macOS, the .align <n> directive aligns to 2^n, not just n.
> > >
> > > I wonder if we should explicitly use '.balign 8', since that always means "align to 8 bytes." Unless, of course, there are platforms that don't support it.
> >
> > This is a general issue that winebuild also explicitly contends with,
> > and it isn't using .balign (nor .p2align).  See
> > tools/winebuild/utils.c:get_alignment().  So, I assume .balign isn't
> > an acceptable solution.  I'd be happy to be told otherwise, though.
>
> It's not supported on Solaris, which was the reason for the winebuild
>  code. I'm not sure if anybody still cares about Solaris nowadays...
>
> -- 
>  Alexandre Julliard
>  [e-mail]
>
> Does this also mean we can kill backticks in shell scripts? ;)

There's no reason to change existing scripts, but we can stop using
backticks in new code and see if anybody complains.