bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Incorrect alignment specifiers in ovr_capi_dynamic.h

RESOLVED FIXED in Firefox 52

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: kip)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: gfx-noted)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
clang-cl emits the following errors when building gfx/vr:

06:16:27     INFO -  c:/builds/moz2_slave/try-w32-st-an-d-00000000000000/build/src/gfx/vr/ovr_capi_dynamic.h(392,16) :  error(clang): requested alignment is less than minimum alignment of 8 for type '(anonymous struct at c:/builds/moz2_slave/try-w32-st-an-d-00000000000000/build/src/gfx/vr/ovr_capi_dynamic.h:392:9)'
06:16:27     INFO -  typedef struct OVR_ALIGNAS(OVR_PTR_SIZE) {
06:16:27     INFO -                 ^
06:16:27     INFO -  c:/builds/moz2_slave/try-w32-st-an-d-00000000000000/build/src/gfx/vr/ovr_capi_dynamic.h(46,24) :  note(clang): expanded from macro 'OVR_ALIGNAS'
06:16:27     INFO -  #define OVR_ALIGNAS(n) alignas(n)
06:16:27     INFO -                         ^
06:16:27     INFO -  c:/builds/moz2_slave/try-w32-st-an-d-00000000000000/build/src/gfx/vr/ovr_capi_dynamic.h(401,16) :  error(clang): requested alignment is less than minimum alignment of 8 for type '(anonymous struct at c:/builds/moz2_slave/try-w32-st-an-d-00000000000000/build/src/gfx/vr/ovr_capi_dynamic.h:401:9)'
06:16:27     INFO -  typedef struct OVR_ALIGNAS(OVR_PTR_SIZE) {
06:16:27     INFO -                 ^
06:16:27     INFO -  c:/builds/moz2_slave/try-w32-st-an-d-00000000000000/build/src/gfx/vr/ovr_capi_dynamic.h(46,24) :  note(clang): expanded from macro 'OVR_ALIGNAS'
06:16:27     INFO -  #define OVR_ALIGNAS(n) alignas(n)
06:16:27     INFO -                         ^
06:16:27     INFO -  c:/builds/moz2_slave/try-w32-st-an-d-00000000000000/build/src/gfx/vr/ovr_capi_dynamic.h(412,16) :  error(clang): requested alignment is less than minimum alignment of 8 for type '(anonymous struct at c:/builds/moz2_slave/try-w32-st-an-d-00000000000000/build/src/gfx/vr/ovr_capi_dynamic.h:412:9)'
06:16:27     INFO -  typedef struct OVR_ALIGNAS(OVR_PTR_SIZE) {
06:16:27     INFO -                 ^
06:16:27     INFO -  c:/builds/moz2_slave/try-w32-st-an-d-00000000000000/build/src/gfx/vr/ovr_capi_dynamic.h(46,24) :  note(clang): expanded from macro 'OVR_ALIGNAS'
06:16:27     INFO -  #define OVR_ALIGNAS(n) alignas(n)
06:16:27     INFO -                         ^

Looking at <https://dxr.mozilla.org/mozilla-central/source/gfx/vr/ovr_capi_dynamic.h?case=true&from=ovr_capi_dynamic.h#392>, for example, in x86 builds, that code will be expanded to:

typedef struct alignas(4) {
  // ... some 4-byte aligned stuff
  double SensorSampleTime;
} ovrLayerEyeFov;

How is this code possibly correct?  Shouldn't the alignment here be 8?
Flags: needinfo?(vladimir)
Whiteboard: gfx-noted
This affects ARM/clang builds as well.
Blocks: 1163171
No longer blocks: 752004
Summary: Incorrect alignment specifiers in ovr_capi_dynamic.h in x86 builds → Incorrect alignment specifiers in ovr_capi_dynamic.h
Kip, can you check this with the latest oculus SDK? Do they still have the same (incorrect) alignment specifiers? If they do, it seems like they're probably just being ignored by other compilers.
Flags: needinfo?(vladimir)
(Assignee)

Updated

2 years ago
Assignee: nobody → kgilbert
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #2)
> Kip, can you check this with the latest oculus SDK? Do they still have the
> same (incorrect) alignment specifiers? If they do, it seems like they're
> probably just being ignored by other compilers.

ni? to kip for this.
Flags: needinfo?(kgilbert)
(Assignee)

Comment 4

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #2)
> > Kip, can you check this with the latest oculus SDK? Do they still have the
> > same (incorrect) alignment specifiers? If they do, it seems like they're
> > probably just being ignored by other compilers.
> 
> ni? to kip for this.
I've checked the latest Oculus SDK and see that the OVR_ALIGNAS macro has been indeed been changed.

As of the Oculus v1.6 SDK:

#if !defined(OVR_ALIGNAS)
    #if defined(__GNUC__) || defined(__clang__)
        #define OVR_ALIGNAS(n) __attribute__((aligned(n)))
    #elif defined(_MSC_VER) || defined(__INTEL_COMPILER)
        #define OVR_ALIGNAS(n) __declspec(align(n))
    #elif defined(__CC_ARM)
        #define OVR_ALIGNAS(n) __align(n)
    #else
        #error Need to define OVR_ALIGNAS
    #endif
#endif

I'll take this bug and update the ovr_capi_dynamic.h file.
Flags: needinfo?(kgilbert)
QA Contact: kgilbert
(In reply to :kip (Kearwood Gilbert) from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #2)
> > > Kip, can you check this with the latest oculus SDK? Do they still have the
> > > same (incorrect) alignment specifiers? If they do, it seems like they're
> > > probably just being ignored by other compilers.
> > 
> > ni? to kip for this.
> I've checked the latest Oculus SDK and see that the OVR_ALIGNAS macro has
> been indeed been changed.
> 
> As of the Oculus v1.6 SDK:
> 
> #if !defined(OVR_ALIGNAS)
>     #if defined(__GNUC__) || defined(__clang__)
>         #define OVR_ALIGNAS(n) __attribute__((aligned(n)))
>     #elif defined(_MSC_VER) || defined(__INTEL_COMPILER)
>         #define OVR_ALIGNAS(n) __declspec(align(n))
>     #elif defined(__CC_ARM)
>         #define OVR_ALIGNAS(n) __align(n)
>     #else
>         #error Need to define OVR_ALIGNAS
>     #endif
> #endif

That alone doesn't fix the problem, because OVR_ALIGNAS is being used with values that are less than the minimum alignment of types on some platforms.  Or, at least OVR_ALIGNAS *was* being used in such a way; did the uses of OVR_ALIGNAS get fixed, too?
Flags: needinfo?(kgilbert)
(Assignee)

Comment 6

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #5)
> (In reply to :kip (Kearwood Gilbert) from comment #4)
> > (In reply to Nathan Froyd [:froydnj] from comment #3)
> > > (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #2)
> > > > Kip, can you check this with the latest oculus SDK? Do they still have the
> > > > same (incorrect) alignment specifiers? If they do, it seems like they're
> > > > probably just being ignored by other compilers.
> > > 
> > > ni? to kip for this.
> > I've checked the latest Oculus SDK and see that the OVR_ALIGNAS macro has
> > been indeed been changed.
> > 
> > As of the Oculus v1.6 SDK:
> > 
> > #if !defined(OVR_ALIGNAS)
> >     #if defined(__GNUC__) || defined(__clang__)
> >         #define OVR_ALIGNAS(n) __attribute__((aligned(n)))
> >     #elif defined(_MSC_VER) || defined(__INTEL_COMPILER)
> >         #define OVR_ALIGNAS(n) __declspec(align(n))
> >     #elif defined(__CC_ARM)
> >         #define OVR_ALIGNAS(n) __align(n)
> >     #else
> >         #error Need to define OVR_ALIGNAS
> >     #endif
> > #endif
> 
> That alone doesn't fix the problem, because OVR_ALIGNAS is being used with
> values that are less than the minimum alignment of types on some platforms. 
> Or, at least OVR_ALIGNAS *was* being used in such a way; did the uses of
> OVR_ALIGNAS get fixed, too?

OVR_ALIGNAS is still being called with "4" as a parameter for both 64-bit and 32-bit builds in the new SDK headers.

Is this something that should be set to ignore?  FWIW, once Bug 1250244 lands, this code will only be active for Windows builds and the "050" version of the header will be deleted.
Flags: needinfo?(kgilbert) → needinfo?(nfroyd)
(In reply to :kip (Kearwood Gilbert) from comment #6)
> OVR_ALIGNAS is still being called with "4" as a parameter for both 64-bit
> and 32-bit builds in the new SDK headers.

Hm, that means the problem would still be there...

> Is this something that should be set to ignore?  FWIW, once Bug 1250244
> lands, this code will only be active for Windows builds and the "050"
> version of the header will be deleted.

...but deactivating that code on non-Windows would certainly help with the Android situation.  Just to clarify, the ovr_* stub headers would never be included on non-Windows?

I guess once bug 1250244 lands, we can work on fixing the original problem in this bug, which is that clang-cl doesn't approve of the alignment specifiers.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 8

2 years ago
Bug 1250244 has now landed and this code will only be included in Windows builds.  Oculus may later add support for Linux and OSX (hopefully), but for now has not announced any plans.

Please advise if we need to do anything else to clean this up.

(In reply to Nathan Froyd [:froydnj] from comment #7)
> (In reply to :kip (Kearwood Gilbert) from comment #6)
> > OVR_ALIGNAS is still being called with "4" as a parameter for both 64-bit
> > and 32-bit builds in the new SDK headers.
> 
> Hm, that means the problem would still be there...
> 
> > Is this something that should be set to ignore?  FWIW, once Bug 1250244
> > lands, this code will only be active for Windows builds and the "050"
> > version of the header will be deleted.
> 
> ...but deactivating that code on non-Windows would certainly help with the
> Android situation.  Just to clarify, the ovr_* stub headers would never be
> included on non-Windows?
> 
> I guess once bug 1250244 lands, we can work on fixing the original problem
> in this bug, which is that clang-cl doesn't approve of the alignment
> specifiers.
Flags: needinfo?(nfroyd)
I haven't tried compiling with clang-cl recently, but the alignment specifiers are still wrong, and clang-cl will complain about them.
Flags: needinfo?(nfroyd)
Have now checked compiling with clang-cl, and it does indeed complain.  Can we get the alignment specifiers adjusted?  I understand that they might be the same as the Oculus SDK, but can we figure out whether MSVC is ignoring them, and remove the specifiers?  Or if MSVC *isn't* ignoring them, that is a bug we can usefully report to clang-cl.
Flags: needinfo?(kgilbert)
(Assignee)

Comment 11

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #10)
> Have now checked compiling with clang-cl, and it does indeed complain.  Can
> we get the alignment specifiers adjusted?  I understand that they might be
> the same as the Oculus SDK, but can we figure out whether MSVC is ignoring
> them, and remove the specifiers?  Or if MSVC *isn't* ignoring them, that is
> a bug we can usefully report to clang-cl.

As long as we maintain binary compatibility with the structures passed to the Oculus runtime, I think it's fine to adjust the alignment specifiers to pass clang-cl.  I would be glad to take this on.
Flags: needinfo?(kgilbert)
Kip, have you looked at this at all?  This is a hard blocked to being able to compile m-c unmodified with clang-cl.
Blocks: 752004
No longer blocks: 1163171
Flags: needinfo?(kgilbert)
(Assignee)

Comment 13

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #12)
> Kip, have you looked at this at all?  This is a hard blocked to being able
> to compile m-c unmodified with clang-cl.

There were some security and crashing bugs I had to get out of the way first.  I'll take some time on this today.
Flags: needinfo?(kgilbert)
(Assignee)

Comment 14

2 years ago
I spent some time today trying to reproduce this.

I've compiled clang-cl and following the directions here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Building_Firefox_on_Windows_with_clang-cl

I found that I needed to apply the patch in Bug 1296737 to build on my Windows 10 box with MSVC 2015.

I have clang-cl builds starting but now failing before hitting the Oculus headers with errors such as:


 2:25.32 c:/dev/mozilla-central/obj-optimized-clancl/dist/include/js/Class.h(937,15):  error: static_assert expression is not an integral constant expression
 2:25.32 static_assert(offsetof(JSClass, flags) == offsetof(Class, flags),
 2:25.32               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2:25.32 C:\Program Files (x86)\Windows Kits\10\include\10.0.10586.0\ucrt\stddef.h(38,31):  note: expanded from macro 'offsetof'
 2:25.32         #define offsetof(s,m) ((size_t)&reinterpret_cast<char const volatile&>((((s*)0)->m)))
 2:25.32                               ^
 2:25.32 c:/dev/mozilla-central/obj-optimized-clancl/dist/include/js/Class.h(937,15):  note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression
 2:25.33 C:\Program Files (x86)\Windows Kits\10\include\10.0.10586.0\ucrt\stddef.h(38,32):  note: expanded from macro 'offsetof'
 2:25.33         #define offsetof(s,m) ((size_t)&reinterpret_cast<char const volatile&>((((s*)0)->m)))


This is perhaps due to:

https://llvm.org/bugs/show_bug.cgi?id=26117

My mozconfig:

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-optimized-clancl
mk_add_options AUTOCLOBBER=1
ac_add_options --target=x86_64-pc-mingw32
ac_add_options --host=x86_64-pc-mingw32

export CC="clang-cl.exe -m64"
export CXX="clang-cl.exe -m64"

Could you share details on your mozconfig and any other setting I may have missed?
Flags: needinfo?(nfroyd)
Ah, interesting!  I haven't tried tackling 64-bit Windows yet.  My mozconfig is merely:

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-optimized-clancl
mk_add_options AUTOCLOBBER=1

export CC="clang-cl.exe"
export CXX="clang-cl.exe"

David Major noted that clang-cl will issue an error about the alignment specifier, but our default is to compile with -fallback, so MSVC then gets a chance to compile any files than clang can't.  So you won't see a hard, build-stopping error.

Sorry about not telling you about bug 1296737; there are a couple of other small patches that would be needed to make a clang-cl build completely succeed: https://pastebin.mozilla.org/8912587.  I expected from comment 10 that some sleuthing with MSVC would be required here, and clang-cl wouldn't be necessary!
Flags: needinfo?(nfroyd)
(Assignee)

Comment 16

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #15)
> Ah, interesting!  I haven't tried tackling 64-bit Windows yet.  My mozconfig
> is merely:
> 
> mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-optimized-clancl
> mk_add_options AUTOCLOBBER=1
> 
> export CC="clang-cl.exe"
> export CXX="clang-cl.exe"
> 
> David Major noted that clang-cl will issue an error about the alignment
> specifier, but our default is to compile with -fallback, so MSVC then gets a
> chance to compile any files than clang can't.  So you won't see a hard,
> build-stopping error.
> 
> Sorry about not telling you about bug 1296737; there are a couple of other
> small patches that would be needed to make a clang-cl build completely
> succeed: https://pastebin.mozilla.org/8912587.  I expected from comment 10
> that some sleuthing with MSVC would be required here, and clang-cl wouldn't
> be necessary!

After applying those patches and attempting a 32-bit build matching your mozconfig, my build is failing
to configure jemalloc.  The build.log:

--- 8< ------ 8< ------ 8< ------ 8< ---

configure:4476: clang-cl.exe -Xclang -std=gnu99 -nologo -o conftest.exe -Zi -MT -W3 -FS -Qunused-arguments -TC -nologo -wd4091 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -FS -wd4244 -wd4267 -wd4819 -Wno-unknown-pragmas -Wno-ignored-pragmas -Wno-deprecated-declarations -Wno-invalid-noreturn -we4553 -Qunused-arguments -Ic:/dev/mozilla-central/memory/jemalloc/src/include/msvc_compat -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH conftest.c  >&5
clang-cl.exe: warning: unknown argument ignored in clang-cl: '-LARGEADDRESSAWARE'
clang-cl.exe: warning: unknown argument ignored in clang-cl: '-NXCOMPAT'
clang-cl.exe: warning: unknown argument ignored in clang-cl: '-SAFESEH'
LINK : conftest.exe not found or not built by the last incremental link; performing full link
configure:4476: $? = 0
configure:4476: ./conftest.exe
configure:4476: $? = 0
configure:4487: result: no
configure:4517: result: Using a predefined value for sizeof(void *): 4 for 32-bit, 8 for 64-bit
configure:4570: checking size of int
configure:4575: clang-cl.exe -Xclang -std=gnu99 -nologo -o conftest.exe -Zi -MT -W3 -FS -Qunused-arguments -TC -nologo -wd4091 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -FS -wd4244 -wd4267 -wd4819 -Wno-unknown-pragmas -Wno-ignored-pragmas -Wno-deprecated-declarations -Wno-invalid-noreturn -we4553 -Qunused-arguments -Ic:/dev/mozilla-central/memory/jemalloc/src/include/msvc_compat -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH conftest.c  >&5
clang-cl.exe: warning: unknown argument ignored in clang-cl: '-LARGEADDRESSAWARE'
clang-cl.exe: warning: unknown argument ignored in clang-cl: '-NXCOMPAT'
clang-cl.exe: warning: unknown argument ignored in clang-cl: '-SAFESEH'
LINK : conftest.exe not found or not built by the last incremental link; performing full link
configure:4575: $? = 0
configure:4575: ./conftest.exe
configure:4575: $? = 0
configure:4589: result: 1084153910
configure:4604: error: Unsupported int size: 1084153910

--- 8< ------ 8< ------ 8< ------ 8< ---

The reported int size is different each time.  Perhaps reporting an uninitialized value?

Have you encountered such a thing?

Are you using pre-built clang-cl or build your own? (I built my own)
Using msvc 2013 or 2015?  (I'm on 2015)

Once I can get these builds going, I should be able to fix up the Oculus header...
Flags: needinfo?(nfroyd)
I built my own clang-cl, and I'm using MSVC 2015.  I have not seen your error, but I haven't build 32-bit recently, and I know there were some changes recently that affected jemalloc configure, and then changes that fixed those problems.  I can try building 32-bit again.

But I'm still puzzled why you're trying to build clang-cl; the problem is that these structures have declspec specifiers that are apparently useless, according to clang-cl.  So we should find out whether MSVC is respecting all of them, probably by constructing testcases like:

typedef struct alignas(4) {
  // ... some 4-byte aligned stuff
  double SensorSampleTime;
} ovrLayerEyeFov;

struct test {
  char toPad;
  ovrLayerEyeFov ovrStruct;
};

size_t f()
{
   return offsetof(test, ovrStruct);
}

and examining the assembly to see whether the alignment is being respected.  For each one:

* if MSVC isn't respecting it, we should remove it, as it's useless;
* if MSVC is respecting it, then we should report that as a bug to clang.

Building clang-cl shouldn't be necessary for any of them.  It might be useful as a spot-check once we're done with the above process, but we can cross that bridge when we come to it.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 18

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #17)
> But I'm still puzzled why you're trying to build clang-cl; the problem is
> that these structures have declspec specifiers that are apparently useless,
> according to clang-cl.  So we should find out whether MSVC is respecting all
> of them, probably by constructing testcases like:
I wish to build with clang-cl and replicate your environment so that I can be sure that the replacement of these alignment specifiers is truly portable and won't fail when running VR hardware in clang-cl builds.

The alignment is sensitive in these structures as they are passed to external libraries.  If the alignment is incorrect, it could result in crashing or even security vulnerabilities.

> 
> typedef struct alignas(4) {
>   // ... some 4-byte aligned stuff
>   double SensorSampleTime;
> } ovrLayerEyeFov;
> 
> struct test {
>   char toPad;
>   ovrLayerEyeFov ovrStruct;
> };
> 
> size_t f()
> {
>    return offsetof(test, ovrStruct);
> }
> 
> and examining the assembly to see whether the alignment is being respected. 
> For each one:
> 
> * if MSVC isn't respecting it, we should remove it, as it's useless;
> * if MSVC is respecting it, then we should report that as a bug to clang.

Microsoft's documentation says that when the value passed to __declspec(align(x)) is less than the natural alignment, it results in packing of the members:

https://msdn.microsoft.com/en-us/library/71kf49f1.aspx

msvc considers it valid to pass in any power-of-two value, including 1 to allow byte-level packing.

> 
> Building clang-cl shouldn't be necessary for any of them.  It might be
> useful as a spot-check once we're done with the above process, but we can
> cross that bridge when we come to it.

I'll check the disassembly to be certain if the alignment specifiers are actually having an effect.

One possibility is that Oculus was including these specifiers to ensure the structure padding is not affected by MSVC's /Zp (Struct Member Alignment) flag.

Once I've fixed the alignment specifiers, I would like to test the resulting clang-cl build on Oculus VR hardward to ensure that it did not regress.
(Assignee)

Comment 19

2 years ago
I did a little experiment comparing clang-cl and msvc:

#include "stdafx.h"

typedef struct alignas(2) {
  int a;
  char b;
  int c;
  char d;
} test_struct;

int main()
{
  return 0;
}

>>>>>>>>>>>>>>>> Result

msvc: warning C4359: '<unnamed-tag>': Alignment specifier is less than actual alignment (4), and will be ignored.
clang-cl: error: requested alignment is less than minimum alignment of 4 for type '(anonymous struct at 

=====


#include "stdafx.h"

typedef struct alignas(2) {
  char a;
  char b;
  char c;
  char d;
} test_struct;

int main()
{
  return 0;
}

>>>>>>>>>>>>>>>> Result

msvc: No warnings or errors
clang-cl: No warnings or errors

=====
(Assignee)

Comment 20

2 years ago
It appears that both clang-cl and msvc are working consistently in both those cases.  I did additional tests to confirm that the behavior is the same with __declspec(align(2)).  I was also able to confirm that in this test that msvc was actually ignoring the alignas(2) when the warning was given.
(Assignee)

Comment 21

2 years ago
Perhaps the difference between clang-cl and msvc is in the default alignment of the particular structures in ovr_capi_dynamic.h, resulting in the error in clang-cl but not in msvc.  In this case, the alignment declaration is effectively acting as a kind of assertion in a strange, brittle, manner.

I'll do some further investigation with the specific structs in ovr_capi_dynamic.h.
(Assignee)

Comment 22

2 years ago
I did another experiment, adding ovr_capi_dynamic.h to a simple project to compile with msvc and clang-cl.

I found that both msvc and clang-cl produced matching warnings and errors:

>>>> msvc
ovr_capi_dynamic.h(458): warning C4359: '<unnamed-tag>': Alignment specifier is less than actual alignment (8), and will be ignored.
ovr_capi_dynamic.h(467): warning C4359: '<unnamed-tag>': Alignment specifier is less than actual alignment (8), and will be ignored.

>>>> clang-cl

./ovr_capi_dynamic.h(458,16):  error: requested alignment is less than minimum alignment of 8 for type '(anonymous struct at ./ovr_capi_dynamic.h:458:9)'
typedef struct OVR_ALIGNAS(OVR_PTR_SIZE) {
               ^
./ovr_capi_dynamic.h(46,24):  note: expanded from macro 'OVR_ALIGNAS'
#define OVR_ALIGNAS(n) alignas(n)
                       ^
./ovr_capi_dynamic.h(467,16):  error: requested alignment is less than minimum alignment of 8 for type '(anonymous struct at ./ovr_capi_dynamic.h:467:9)'
typedef struct OVR_ALIGNAS(OVR_PTR_SIZE) {
               ^
./ovr_capi_dynamic.h(46,24):  note: expanded from macro 'OVR_ALIGNAS'
#define OVR_ALIGNAS(n) alignas(n)
(Assignee)

Comment 23

2 years ago
Recently Oculus has released an updated SDK, version 1.8.  They have added some pragma's to their headers to ignore the warnings but have not removed the OVR_ALIGNAS(4) specifiers:

#if defined(_MSC_VER)
    #pragma warning(push)
    #pragma warning(disable: 4324) // structure was padded due to __declspec(align())
    #pragma warning(disable: 4359) // The alignment specified for a type is less than the alignment of the type of one of its data members
#endif
(Assignee)

Comment 24

2 years ago
The headers from Oculus 1.8 SDK also included a new OVR_ALIGNAS macro:

----8<--------8<--------8<--------8<----

#if !defined(OVR_ALIGNAS)
#if defined(__GNUC__) || defined(__clang__)
#define OVR_ALIGNAS(n) __attribute__((aligned(n)))
#elif defined(_MSC_VER) || defined(__INTEL_COMPILER)
#define OVR_ALIGNAS(n) __declspec(align(n))
#elif defined(__CC_ARM)
#define OVR_ALIGNAS(n) __align(n)
#else
#error Need to define OVR_ALIGNAS
#endif
#endif

----8<--------8<--------8<--------8<----

Updating this macro in ovr_capi_dynamic.h appears to eliminate the errors returned by clang-cl.

I'll put together a patch with this change.  If this enables you to produce a build with clang-cl, I'd be glad to test it out on real hardware.

Thanks!!
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8795927 - Flags: review?(nfroyd)
(Assignee)

Comment 26

2 years ago
The attached patch includes the minimal change to eliminate the errors returned by clang-cl.

I have opened Bug 1305873 to cover updating the rest of the header as we link to the newer Oculus SDK runtime.

Comment 27

2 years ago
mozreview-review
Comment on attachment 8795927 [details]
Bug 1255210 - Update OVR_ALIGNAS macro in ovr_capi_dynamic.h

https://reviewboard.mozilla.org/r/81892/#review80750

This works great, thank you!
Attachment #8795927 - Flags: review?(nfroyd) → review+

Comment 28

2 years ago
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/878008bf7bec
Update OVR_ALIGNAS macro in ovr_capi_dynamic.h r=froydnj

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/878008bf7bec
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.