Ticket #37833

mingw-get std C++11 issues with GCC 6.4

Date d'ouverture: 2017-12-24 04:39 Dernière mise à jour: 2018-01-28 06:21

Ouvert [Owner assigned]
5 - moyen
5 - moyen
Score: 0
No votes
0.0% (0/0)
0.0% (0/0)


Per Slack conversation Keith states he has some patches for mingw-get and plans to share here.

Ticket History (3/12 Histories)

2017-12-24 04:39 Updated by: earnie
  • New Ticket "mingw-get std C++11 issues with GCC 6.4" created
2018-01-21 06:45 Updated by: keith

I notice you declined to actually describe the issues, to which this ticket relates. I assume that you are referring to mingw-get-earnie-v1.patch; I think we need to break that down into logically separate issues, which we can then address through individual comments on this ticket.

2018-01-27 04:03 Updated by: keith

Before I follow up on your specific issues, let's examine a more fundamental one which arises from using any version of GCC later than GCC-3.4.5, (which is what I've been using until now, to build the mingw-get code base). Using this elderly version of GCC introduces no DLL dependencies, (other than on standard system DLLs, which should already be present on any end user's machine); moving to a later GCC version introduces additional dependencies on GCC/G++ specific DLLs, (which we cannot assume would be present on an end user's machine), and would thus complicate the initial mingw-get delivery process.

To circumvent this potential issue, before we move to any more recent GCC version, we must adapt the build system to avoid these additional DLL dependencies; the attached static-linking.patch achieves this, by adding the -static-libgcc and -static-libstdc++ options to the gcc and g++ commands, as appropriate.

One drawback, arising from this stratagem, is that GCC-3.4.5 doesn't support the -static-libstdc++ option. Thus, if I continue to build with GCC-3.4.5, every g++ compilation results in an unrecognised option ... warning. However, it does seem to be just a warning, and doesn't appear to interrupt the build -- it doesn't cause g++ to terminate with any abnormal completion status code -- so perhaps it isn't worth the effort of trying to suppress the warning.

2018-01-27 07:11 Updated by: keith

Now, let's examine one of the logically distinct issues within your composite patch:

* dllhook.cpp (MESSAGE_INTERNAL_ERROR): Resolve complaints from std
C++11 requires a space between a literal and string macro.

* pkgexec.cpp (LUA_LIBEXEC_PATH): Ditto.
Miscellaneous trailing whitespace removal.

Compiling with GCC-3.4.5, this issue doesn't arise; (IIRC, it default's to std=C++98, which doesn't require this level of pedantry). Similarly, it wouldn't arise if we compiled for an earlier C++ standard, with a newer GCC. Nonetheless, there's no reason why this should be compiled for an earlier C++ standard, so I've broken out c++11-string-conflation.patch.

Do please note that, in the break-out process, I've modified your ChangeLog entry, to correctly reflect that it's the assignment of LUA_PATH, and not the definition of LUA_LIBEXEC_PATH, that has been adjusted in pkgexec.cpp. I've also deleted the reference to removal of trailing white space; that's such a trivial change, with no effect whatsoever on compilation, that I don't consider it to be worthy of mention.

2018-01-27 23:11 Updated by: keith

The next of your issues, which I'd like to consider, is:

* mkpath.cpp (stat): Use _stat instead.
Miscellaneous trailing whitespace removal.

Once again, I hardly think the trailing white space adjustment is worth a mention, and on its own, it certainly doesn't warrant an edit. Beyond that, (ignoring what appear to be ChangeLog typographic errors -- the file in question is mkpath.c, not mkpath.cpp, and the reference to stat is ambiguous w.r.t. struct stat vs. function stat), I simply don't understand why this change should be necessary ... unless you go out of your way to define _NO_OLDNAMES, our sys/stat.h defines (POSIX compatible) struct stat to be identical to Microsoft's ugly, non-standard struct _stat.

Sorry, but I am inclined to reject this change, (on the grounds that I will not unnecessarily adopt Microsoft lock-in coding standards, and without this change mkpath.c is potentially portable).

2018-01-28 01:37 Updated by: keith

Compiling with GCC-3.4.5, I did not encounter this:

* pkgkeys.h (EXTERN_C_DECL): Don't declare extern "C" since this is all
C++ and compile of setup.cpp complained about it.

If I compile with GCC-6.3.0, instead of with GCC-3.4.5, I do see an issue. However, I disagree with your interpretation, and your solution. The declarations in pkgkeys.h refer to C-string constants, defined in pkgkeys.c -- so, not C++ -- and as such, must retain extern "C" linkage. IMO, the correct solution is to ensure that this linkage attribute is preserved for the locally reproduced definitions of the three keywords which appear in setup.cpp; the attached setup-keywords.patch resolves the issue, for me.

2018-01-28 06:21 Updated by: keith

The final issue, conflated into your patch, is:

* Makefile.in (CPPFLAGS): (__MSVCRT_VERSION__) Define for __utimebuf64.
(_USE_32BIT_TIME_T): Define for __utimebuf32.
(LIBS): Add -lmsvcr80 for _stat64 and _stat32.

I think we can all agree, pursuant to discussion in our Slack #general channel, that the solution implemented by your patch would be the wrong thing to do. I agree that there is an issue, arising from a reference to the _utime64() function, and its associated struct __utimebuf64 data type, (or alternatively to explicit use of the struct __utimebuf32 data type), in tarproc.cpp, but we need to find a better solution than this.

My initial thought was to simply revert the section of the tarproc.cpp implementation, where these dependencies were introduced, to its prior state, (so depending only on function utime() and its associated struct utimebuf data type; indeed, this could be a viable short term work-around, as implemented in attached tarproc-utime.patch). However, in the long term, I think it would be better to address this issue as part of an overall review of the mingwrt time related API. (I already have a patch for this particular issue, which works for Win32, but likely needs some rework to accommodate Win64). That's a subject for another ticket, (which I will raise in due course).


Please login to add comment to this ticket » Connexion