|
From: David Grayson <davidegrayson@gm...> - 2017-04-25 14:55:42
Attachments:
guid-selectany.patch
|
If __declspec(selectany) is not used on the prototype but later used on a definition, GCC seems to ignore it, and you can get multiple-definition errors at link time. That situation can arise in code like Microsoft's usbview utility that has multiple translation units including the following headers: #include <windows.h> #include <initguid.h> #include <winioctl.h> When I have two files that include those header files in that order and I try to compile them with "i686-w64-mingw32-gcc main.c other.c -o test.exe", I get a bunch of errors like this: /home/david/tmp/ccB3GiUW.o:other.c:(.rdata+0x0): multiple definition of `GUID_DEVINTERFACE_DISK' /home/david/tmp/ccRH9Bjf.o:main.c:(.rdata+0x0): first defined here /home/david/tmp/ccB3GiUW.o:other.c:(.rdata+0x10): multiple definition of `GUID_DEVINTERFACE_CDROM' /home/david/tmp/ccRH9Bjf.o:main.c:(.rdata+0x10): first defined here This patch fixes that by adding __declspec(selectany) to the GUID declarations. Thanks! --David Grayson |
|
From: Liu Hao <lh_mouse@12...> - 2017-05-02 09:56:32
|
Kai, did you mean this patch was ok for master? -- Best regards, LH_Mouse |
|
From: Kai Tietz <ktietz70@go...> - 2017-05-02 10:39:03
|
I think, it is worth a try. In general it looks sensible. Kai 2017-05-02 11:56 GMT+02:00 Liu Hao <lh_mouse@...>: > Kai, did you mean this patch was ok for master? > > -- > Best regards, > LH_Mouse > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Mingw-w64-public mailing list > Mingw-w64-public@... > https://lists.sourceforge.net/lists/listinfo/mingw-w64-public |
|
From: Liu Hao <lh_mouse@12...> - 2017-05-02 11:08:23
|
On 2017/5/2 18:38, Kai Tietz wrote: > I think, it is worth a try. In general it looks sensible. As you wish. -- Best regards, LH_Mouse |
|
From: Mateusz <mateuszb@po...> - 2017-05-02 22:43:56
|
W dniu 2017-05-02 o 13:08, Liu Hao pisze:
> On 2017/5/2 18:38, Kai Tietz wrote:
>> I think, it is worth a try. In general it looks sensible.
> As you wish.
For GCC 7.1 and 6.3 the patch is OK.
For GCC 5.4 is not -- error compiling ffmpeg:
CC libavdevice/dshow.o
In file included from f:\msys\m64-54\x86_64-w64-mingw32\include\combaseapi.h:156:0,
from f:\msys\m64-54\x86_64-w64-mingw32\include\objbase.h:14,
from f:\msys\m64-54\x86_64-w64-mingw32\include\ole2.h:17,
from f:\msys\m64-54\x86_64-w64-mingw32\include\ocidl.h:12,
from f:\msys\m64-54\x86_64-w64-mingw32\include\olectl.h:15,
from f:\msys\m64-54\x86_64-w64-mingw32\include\dshow.h:28,
from libavdevice/dshow_capture.h:33,
from libavdevice/dshow.c:22:
libavdevice/dshow.c: In function 'dshow_add_device':
f:\msys\m64-54\x86_64-w64-mingw32\include\uuids.h:228:1: error: 'FORMAT_VideoInfo':'selectany' attribute applies only to initial
ized objects
OUR_GUID_ENTRY(FORMAT_VideoInfo,0x05589f80,0xc356,0x11ce,0xbf,0x01,0x00,0xaa,0x00,0x55,0x59,0x5a)
^
f:\msys\m64-54\x86_64-w64-mingw32\include\uuids.h:229:1: error: 'FORMAT_VideoInfo2':'selectany' attribute applies only to initia
lized objects
OUR_GUID_ENTRY(FORMAT_VideoInfo2,0xf72a76A0,0xeb0a,0x11d0,0xac,0xe4,0x00,0x00,0xc0,0xcc,0x16,0xba)
^
f:\msys\m64-54\x86_64-w64-mingw32\include\uuids.h:230:1: error: 'FORMAT_WaveFormatEx':'selectany' attribute applies only to init
ialized objects
OUR_GUID_ENTRY(FORMAT_WaveFormatEx,0x05589f81,0xc356,0x11ce,0xbf,0x01,0x00,0xaa,0x00,0x55,0x59,0x5a)
^
|
|
From: Liu Hao <lh_mouse@12...> - 2017-05-03 01:34:04
|
On 2017/5/3 6:23, Mateusz wrote: > W dniu 2017-05-02 o 13:08, Liu Hao pisze: >> On 2017/5/2 18:38, Kai Tietz wrote: >>> I think, it is worth a try. In general it looks sensible. >> As you wish. > > For GCC 7.1 and 6.3 the patch is OK. > > For GCC 5.4 is not -- error compiling ffmpeg: > CC libavdevice/dshow.o > In file included from f:\msys\m64-54\x86_64-w64-mingw32\include\combaseapi.h:156:0, > from f:\msys\m64-54\x86_64-w64-mingw32\include\objbase.h:14, > from f:\msys\m64-54\x86_64-w64-mingw32\include\ole2.h:17, > from f:\msys\m64-54\x86_64-w64-mingw32\include\ocidl.h:12, > from f:\msys\m64-54\x86_64-w64-mingw32\include\olectl.h:15, > from f:\msys\m64-54\x86_64-w64-mingw32\include\dshow.h:28, > from libavdevice/dshow_capture.h:33, > from libavdevice/dshow.c:22: > libavdevice/dshow.c: In function 'dshow_add_device': > f:\msys\m64-54\x86_64-w64-mingw32\include\uuids.h:228:1: error: 'FORMAT_VideoInfo':'selectany' attribute applies only to initial > ized objects > OUR_GUID_ENTRY(FORMAT_VideoInfo,0x05589f80,0xc356,0x11ce,0xbf,0x01,0x00,0xaa,0x00,0x55,0x59,0x5a) > ^ > f:\msys\m64-54\x86_64-w64-mingw32\include\uuids.h:229:1: error: 'FORMAT_VideoInfo2':'selectany' attribute applies only to initia > lized objects > OUR_GUID_ENTRY(FORMAT_VideoInfo2,0xf72a76A0,0xeb0a,0x11d0,0xac,0xe4,0x00,0x00,0xc0,0xcc,0x16,0xba) > ^ > f:\msys\m64-54\x86_64-w64-mingw32\include\uuids.h:230:1: error: 'FORMAT_WaveFormatEx':'selectany' attribute applies only to init > ialized objects > OUR_GUID_ENTRY(FORMAT_WaveFormatEx,0x05589f81,0xc356,0x11ce,0xbf,0x01,0x00,0xaa,0x00,0x55,0x59,0x5a) > ^ Kai, should we revert it? We have to deal with the multiple definition error thereafter. -- Best regards, LH_Mouse |
|
From: Liu Hao <lh_mouse@12...> - 2017-05-03 02:49:35
|
On 2017/5/3 9:33, Liu Hao wrote: > Kai, should we revert it? We have to deal with the multiple definition > error thereafter. > A number of UUIDs/GUIDs are suffering from such multiple definitions, which can be discovered using the following commands: ```bash grep -EhrI '^DEFINE_(GUID|OLEGUID)' | sed -r 's@^DEFINE_(GUID|OLEGUID)\s*\((\w+)\s*,.*$@\2@' | sort | tee uuids-all.txt | uniq > uuids-uniq.txt diff -U0 uuids-all.txt uuids-uniq.txt | grep -v '^@@' ``` This outputs 106 redundant definitions at the moment. -- Best regards, LH_Mouse |
|
From: David Grayson <davidegrayson@gm...> - 2017-05-03 04:46:18
|
I won't be offended if you revert it. I'm not sure what your shell commands do, but as long as there is one GUID defined in one header file, and the selectany attribute is not used properly, you can get multiple definition errors, because that header file could be used in multiple translation units in conjunction with initguid.h. It sounds like guiddef.h should probably check the GCC major version. Based on the tests people have reported in this thread, I would guess that for GCC 6 and above the selectany attribute on the declaration is required, while on GCC 5 and below the selectany attribute on the declaration is forbidden. Or we could get fancy and add a test to the configure script to figure out which is the case. --David Grayson On Tue, May 2, 2017 at 7:49 PM, Liu Hao <lh_mouse@...> wrote: > On 2017/5/3 9:33, Liu Hao wrote: > > Kai, should we revert it? We have to deal with the multiple definition > > error thereafter. > > > A number of UUIDs/GUIDs are suffering from such multiple definitions, > which can be discovered using the following commands: > > ```bash > grep -EhrI '^DEFINE_(GUID|OLEGUID)' | > sed -r 's@^DEFINE_(GUID|OLEGUID)\s*\((\w+)\s*,.*$@\2@' | > sort | > tee uuids-all.txt | > uniq > uuids-uniq.txt > diff -U0 uuids-all.txt uuids-uniq.txt | grep -v '^@@' > ``` > > This outputs 106 redundant definitions at the moment. > > -- > Best regards, > LH_Mouse > > > ------------------------------------------------------------ > ------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Mingw-w64-public mailing list > Mingw-w64-public@... > https://lists.sourceforge.net/lists/listinfo/mingw-w64-public > |
|
From: Liu Hao <lh_mouse@12...> - 2017-05-03 05:27:31
|
On 2017/5/3 12:46, David Grayson wrote: > I won't be offended if you revert it. I reverted it and added a line of comment explaining what had happened. > I'm not sure what your shell commands do, but as long as there is one GUID > defined in one header file, and the selectany attribute is not used > properly, you can get multiple definition errors, because that header file > could be used in multiple translation units in conjunction with initguid.h. Those definitions are not in effect unless the macro `INITGUID` is defined, which is the case for our users. > It sounds like guiddef.h should probably check the GCC major version. > Based on the tests people have reported in this thread, I would guess that > for GCC 6 and above the selectany attribute on the declaration is required, > while on GCC 5 and below the selectany attribute on the declaration is > forbidden. Or we could get fancy and add a test to the configure script to > figure out which is the case. Both require citation from GCC documentation. But neither is an optimal solution IMHO. I suggest we ensure each GUID is defined only once then remove that attribute completely. -- Best regards, LH_Mouse |
|
From: David Grayson <davidegrayson@gm...> - 2017-05-03 15:12:56
|
> > > It sounds like guiddef.h should probably check the GCC major version. > > Based on the tests people have reported in this thread, I would guess > that > > for GCC 6 and above the selectany attribute on the declaration is > required, > > while on GCC 5 and below the selectany attribute on the declaration is > > forbidden. Or we could get fancy and add a test to the configure script > to > > figure out which is the case. > Both require citation from GCC documentation. But neither is an optimal > solution IMHO. I suggest we ensure each GUID is defined only once then > remove that attribute completely. > When you say "optimal", what are you optimizing for? I'm trying to optimize for being able to port code from Visual Studio to mingw-w64. Microsoft has example code (UsbView) that has a header file that includes the windows.h, followed by initguid.h, followed by winioctl.h. You can see the header file here: https://github.com/Microsoft/Windows-driver-samples/blob/master/usb/usbview/uvcview.h The UsbView code uses that header file in several different translation units, so you end up having a bunch of duplicate GUID definitions for all the GUIDs defined in winioctl.h, like GUID_DEVINTERFACE_DISK. To prevent multiple definition errors, the Microsoft header files using the selectany attribute. I don't know why Microsoft engineered such a complicated solution for defining GUIDs, but they did, and I would think we should try to make our header files compatible with it. If it just takes a few preprocessor if statements that check the GCC version, that seems OK. --David |
|
From: Liu Hao <lh_mouse@12...> - 2017-05-03 15:49:26
|
On 2017/5/3 23:12, David Grayson wrote: > When you say "optimal", what are you optimizing for? > For decreasing object file sizes and link time, apparently. > The UsbView code uses that header file in several different translation > units, so you end up having a bunch of duplicate GUID definitions for all > the GUIDs defined in winioctl.h, like GUID_DEVINTERFACE_DISK. To prevent > multiple definition errors, the Microsoft header files using the selectany > attribute. Similar solutions are used for C++ template specializations (functions or classes) and `inline` functions (note that C++ `inline` is different from C `inline` be it C99 or GNU89). > I don't know why Microsoft engineered such a complicated solution for > defining GUIDs, but they did, and I would think we should try to make our > header files compatible with it. If it just takes a few preprocessor if > statements that check the GCC version, that seems OK. I have no idea either, but the case here is abnormal: If the macro `INITGUID` is *not* defined before `#include`ing _guiddef.h_, those GUIDs/UUIDs are declared (instead of defined) only as `extern const` objects. Our behavior is hence consistent with Microsoft's own headers (I only have VS2017 at hand and I have examined it). If your conclusion > If __declspec(selectany) is not used on the prototype but later used on a definition, GCC seems to ignore it, and you can get multiple-definition errors at link time. is true, then you should first try adding `-DINITGUID` in your `CPPFLAGS`, effectively forcing defining every GUID in your project. If it does solve the problem, you can check the pre-processed source files produced by _both MSVC and GCC_ (without that `-E` option) to see where the macro `INITGUID` is defined. If it turns out that a non-selectany declaration followed by a selectany definition is causing linker errors, it is probably a GCC bug and not a MinGW-w64 bug and we are unable to fix it after all. -- Best regards, LH_Mouse |
|
From: David Grayson <davidegrayson@gm...> - 2017-05-03 15:19:26
|
By the way, it's really not clear to me what specific solution you are proposing when you say "ensure each GUID is defined only once" but I'm imagining it will introduce new incompatibilities/quirks that people have to worry about when switching between Visual Studio and mingw-w64, because you would have to prevent any GUID definitions from being emitted by header files. --David On Wed, May 3, 2017 at 8:12 AM, David Grayson <davidegrayson@...> wrote: > > It sounds like guiddef.h should probably check the GCC major version. >> > Based on the tests people have reported in this thread, I would guess >> that >> > for GCC 6 and above the selectany attribute on the declaration is >> required, >> > while on GCC 5 and below the selectany attribute on the declaration is >> > forbidden. Or we could get fancy and add a test to the configure script >> to >> > figure out which is the case. >> Both require citation from GCC documentation. But neither is an optimal >> solution IMHO. I suggest we ensure each GUID is defined only once then >> remove that attribute completely. >> > > When you say "optimal", what are you optimizing for? > > I'm trying to optimize for being able to port code from Visual Studio to > mingw-w64. Microsoft has example code (UsbView) that has a header file > that includes the windows.h, followed by initguid.h, followed by > winioctl.h. You can see the header file here: > > https://github.com/Microsoft/Windows-driver-samples/blob/ > master/usb/usbview/uvcview.h > > The UsbView code uses that header file in several different translation > units, so you end up having a bunch of duplicate GUID definitions for all > the GUIDs defined in winioctl.h, like GUID_DEVINTERFACE_DISK. To prevent > multiple definition errors, the Microsoft header files using the selectany > attribute. > > I don't know why Microsoft engineered such a complicated solution for > defining GUIDs, but they did, and I would think we should try to make our > header files compatible with it. If it just takes a few preprocessor if > statements that check the GCC version, that seems OK. > > --David > |
|
From: Mateusz <mateuszb@po...> - 2017-05-03 21:13:27
Attachments:
selectany.diff
|
We could consider define DECLSPEC_SELECTANY to __attribute__((weak)) for old GCC. Patch for this attached. W dniu 2017-05-03 o 17:19, David Grayson pisze: > By the way, it's really not clear to me what specific solution you are > proposing when you say "ensure each GUID is defined only once" but I'm > imagining it will introduce new incompatibilities/quirks that people have > to worry about when switching between Visual Studio and mingw-w64, because > you would have to prevent any GUID definitions from being emitted by header > files. > > --David > > On Wed, May 3, 2017 at 8:12 AM, David Grayson <davidegrayson@...> > wrote: > >>> It sounds like guiddef.h should probably check the GCC major version. >>>> Based on the tests people have reported in this thread, I would guess >>> that >>>> for GCC 6 and above the selectany attribute on the declaration is >>> required, >>>> while on GCC 5 and below the selectany attribute on the declaration is >>>> forbidden. Or we could get fancy and add a test to the configure script >>> to >>>> figure out which is the case. >>> Both require citation from GCC documentation. But neither is an optimal >>> solution IMHO. I suggest we ensure each GUID is defined only once then >>> remove that attribute completely. >>> >> >> When you say "optimal", what are you optimizing for? >> >> I'm trying to optimize for being able to port code from Visual Studio to >> mingw-w64. Microsoft has example code (UsbView) that has a header file >> that includes the windows.h, followed by initguid.h, followed by >> winioctl.h. You can see the header file here: >> >> https://github.com/Microsoft/Windows-driver-samples/blob/ >> master/usb/usbview/uvcview.h >> >> The UsbView code uses that header file in several different translation >> units, so you end up having a bunch of duplicate GUID definitions for all >> the GUIDs defined in winioctl.h, like GUID_DEVINTERFACE_DISK. To prevent >> multiple definition errors, the Microsoft header files using the selectany >> attribute. >> >> I don't know why Microsoft engineered such a complicated solution for >> defining GUIDs, but they did, and I would think we should try to make our >> header files compatible with it. If it just takes a few preprocessor if >> statements that check the GCC version, that seems OK. >> >> --David >> > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Mingw-w64-public mailing list > Mingw-w64-public@... > https://lists.sourceforge.net/lists/listinfo/mingw-w64-public > |
|
From: Mateusz <mateuszb@po...> - 2017-05-03 23:07:52
|
W dniu 2017-05-03 o 23:14, Mateusz pisze: > We could consider define DECLSPEC_SELECTANY to __attribute__((weak)) for old GCC. > > Patch for this attached. Sorry for this patch -- it's not working for old GCC. |
|
From: David Grayson <davidegrayson@gm...> - 2017-05-04 04:19:46
|
Hello, Mateusz, LH_Mouse. Another point against my patch and Mateusz's patch is that applying __declspec(selectany) to a declaration is "incorrect" according to MSDN: https://msdn.microsoft.com/en-us/library/5tkz6s71.aspx The clang people have some handy notes in their test suite about how clang and MSVC treat selectany: https://github.com/llvm-mirror/clang/blob/master/test/SemaCX X/attr-selectany.cpp I tested Visual Studio 2017 and GCC 6.3.0 (from MSYS2. for x86_64) today to see if they have the multiple definition error that caused me to originally post my patch, where selectany on a variable definition is ignored if the variable has a previous declaration without selectany. I tested both C and C++ source files. The results were: GCC with C: has the issue GCC with C++: works Visual Studio with C: works Visual Studio with C++ source: works So now I agree with LH_Mouse that what we are seeing is an issue with GCC and the GCC maintainers should fix it. I wouldn't call it a bug because I don't think there is a standard for non-standard language extensions like __declspec. I disagree with LH_Mouse's absolutist attitude that "we are unable to fix it afterall". I think there are other places where mingw-w64 works around quirks or missing features in GCC. Per LH_Mouse's suggestion, I tried using -DINITGUID with GCC 6.3.0 in my own build environment to compile UsbView. However, that does not work because you get multiple definition errors. GCC, like MSVC, does not let you have multiple definitions of a variable in the same transation unit even if they are marked with selectany. I guess, for now, people using GCC 6+ can apply my patch to their mingw if they care, and eventually maybe we can fix GCC so it's not necessary. --David On Wed, May 3, 2017 at 2:14 PM, Mateusz <mateuszb@...> wrote: > We could consider define DECLSPEC_SELECTANY to __attribute__((weak)) for > old GCC. > > Patch for this attached. > > W dniu 2017-05-03 o 17:19, David Grayson pisze: > > By the way, it's really not clear to me what specific solution you are > > proposing when you say "ensure each GUID is defined only once" but I'm > > imagining it will introduce new incompatibilities/quirks that people have > > to worry about when switching between Visual Studio and mingw-w64, > because > > you would have to prevent any GUID definitions from being emitted by > header > > files. > > > > --David > > > > On Wed, May 3, 2017 at 8:12 AM, David Grayson <davidegrayson@...> > > wrote: > > > >>> It sounds like guiddef.h should probably check the GCC major version. > >>>> Based on the tests people have reported in this thread, I would guess > >>> that > >>>> for GCC 6 and above the selectany attribute on the declaration is > >>> required, > >>>> while on GCC 5 and below the selectany attribute on the declaration is > >>>> forbidden. Or we could get fancy and add a test to the configure > script > >>> to > >>>> figure out which is the case. > >>> Both require citation from GCC documentation. But neither is an optimal > >>> solution IMHO. I suggest we ensure each GUID is defined only once then > >>> remove that attribute completely. > >>> > >> > >> When you say "optimal", what are you optimizing for? > >> > >> I'm trying to optimize for being able to port code from Visual Studio to > >> mingw-w64. Microsoft has example code (UsbView) that has a header file > >> that includes the windows.h, followed by initguid.h, followed by > >> winioctl.h. You can see the header file here: > >> > >> https://github.com/Microsoft/Windows-driver-samples/blob/ > >> master/usb/usbview/uvcview.h > >> > >> The UsbView code uses that header file in several different translation > >> units, so you end up having a bunch of duplicate GUID definitions for > all > >> the GUIDs defined in winioctl.h, like GUID_DEVINTERFACE_DISK. To > prevent > >> multiple definition errors, the Microsoft header files using the > selectany > >> attribute. > >> > >> I don't know why Microsoft engineered such a complicated solution for > >> defining GUIDs, but they did, and I would think we should try to make > our > >> header files compatible with it. If it just takes a few preprocessor if > >> statements that check the GCC version, that seems OK. > >> > >> --David > >> > > ------------------------------------------------------------ > ------------------ > > Check out the vibrant tech community on one of the world's most > > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > > _______________________________________________ > > Mingw-w64-public mailing list > > Mingw-w64-public@... > > https://lists.sourceforge.net/lists/listinfo/mingw-w64-public > > > > > ------------------------------------------------------------ > ------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Mingw-w64-public mailing list > Mingw-w64-public@... > https://lists.sourceforge.net/lists/listinfo/mingw-w64-public > > |