katepanping / libyuv

Automatically exported from code.google.com/p/libyuv
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

__declspec(align()) is illegal on functions, fatal error on VS 2015 #422

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
VS 2015 gives hundreds of errors about illegal use of __declspec(align) on 
function declarations. It appears that this was never supported:
https://msdn.microsoft.com/en-us/library/83ythb65.aspx
and it is now a fatal error. The error messages look like this:

src\third_party\libyuv\source\row_win.cc(4640): error C3323: 'alignas'
and '__declspec(align)' are not allowed on function declarations

This is blocking the porting of Chrome to VS 2015.

The errors show up in row_win.cc, scale_win.cc, compare_win.cc, and
possibly other source files.

Original issue reported on code.google.com by brucedaw...@chromium.org on 8 Apr 2015 at 12:48

GoogleCodeExporter commented 9 years ago
Testing with VS 2013 shows that functions are normally/often aligned to 16 
bytes, regardless of whether they are preceded by __declspec(align(16)). Adding 
__declspec(align(32)) also makes no difference.

Original comment by brucedaw...@chromium.org on 8 Apr 2015 at 12:52

GoogleCodeExporter commented 9 years ago
Theres a 1 cycle penalty if a branch target instruction spans a cache line on 
older CPUs and compilers.
But not a big deal any more, so we can probably go ahead and remove it.

Prefer a way to test it.  Guess I'll check it in and have someone verify later.
It will require a chromium deps roll.

Original comment by fbarch...@chromium.org on 8 Apr 2015 at 2:47

GoogleCodeExporter commented 9 years ago
From VS 2010 to VS 2015 I see no evidence that __declspec(align()) makes any 
difference to code alignment. I see 16-byte alignment whether the requested 
alignment is 0, 16, or 32. If __declspec(align()) does make a difference then 
macro below can be used. But if __declspec(align()) makes no difference then it 
should be deleted and there will be no penalty.

#if _MSC_VER < 1900
#define ALIGNCODE __declspec(align(32))
#else
#define ALIGNCODE
#endif

Original comment by brucedaw...@google.com on 8 Apr 2015 at 4:49

GoogleCodeExporter commented 9 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=1122900 where I ran into this 
issue when building Gecko with VS2015, and where I wrote the (trivial) patch to 
do it. I've attached the same patch here so that it is subject to my 
contributor agreement. Note that the patch is a patch against Firefox, but you 
should be able to apply it to Chromium with the appropriate "patch -p" 
invocation.

Original comment by brian@briansmith.org on 8 Apr 2015 at 10:46

Attachments:

GoogleCodeExporter commented 9 years ago
Note: Brian's patch against Mozilla is against our last import of libyuv, which 
is rev 971 (though likely it will apply with fuzzing/offsets)

Original comment by randell....@gmail.com on 9 Apr 2015 at 5:43

GoogleCodeExporter commented 9 years ago
r1365 removes all code declspec(align(16))

Original comment by fbarch...@chromium.org on 13 Apr 2015 at 12:01

GoogleCodeExporter commented 9 years ago
Almost...

The changes have rolled into Chrome but row_win.cc was missed. It contains 
three __declspec(align(32)) statements.

I'd mark the bug as unfixed but I don't seem to have permissions.

Original comment by brucedaw...@chromium.org on 16 Apr 2015 at 8:22

GoogleCodeExporter commented 9 years ago
right.  changed to unfixed.  fix coming.

Original comment by fbarch...@chromium.org on 20 Apr 2015 at 5:41

GoogleCodeExporter commented 9 years ago
r1374 fixes libyuv end of it.
This is the roll CL for chromium
https://codereview.chromium.org/1094223002/

Original comment by fbarch...@google.com on 21 Apr 2015 at 5:43

GoogleCodeExporter commented 9 years ago
r1374 rolled into chromium... should fix vs2015 build.

Original comment by fbarch...@google.com on 28 Apr 2015 at 1:11