bugmake - Bugs: bug #40227, Various fixes for MSVC build of 4.0

 
 

bug #40227: Various fixes for MSVC build of 4.0

Submitted by:  Christian Boos <cboos>
Submitted on:  Wed 09 Oct 2013 11:42:35 PM UTC  
 
Severity: 3 - NormalItem Group: Build/Install
Status: FixedPrivacy: Public
Assigned to: NoneOpen/Closed: Closed
Component Version: 4.0Operating System: MS Windows
Fixed Release: SCMTriage Status: None

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Wed 23 Oct 2013 04:59:16 PM UTC, comment #21:

OK, I've now applied the /STACK patch, and I'm closing this bug.

Eli Zaretskii <eliz>
Project Member
Tue 22 Oct 2013 09:47:49 PM UTC, comment #20:

Thanks for applying!

As for the /STACK patch, the issue I got in my early tries was an exception with code 0xC00000FD (STATUS_STACK_OVERFLOW). Now my problem is that I can't reproduce it anymore, even with x64 bits and the "default" /STACK.

But googling for "make 0xC00000FD" shows quite a lot of such similar issues, so I'm not (wasn't) alone with this ;-)

In my patch, 0x4000000 was indeed probably excessive and setting it to 0x400000 should be enough as it's 4 times the default. Note that if I lower this value instead (e.g. to 0x80000) I can again trigger a crash but this time with a less informative 0c0000005 exception code (STATUS_ACCESS_VIOLATION).

Christian Boos <cboos>
Fri 18 Oct 2013 10:32:50 AM UTC, comment #19:

I installed changes for all of the problems, with the single exception of the stack size for 64-bit builds. For the latter problem, I would like to see the details: how does the problem manifest itself, and why did you need such a large increase in the stack size (67 MB if I'm not mistaken).

Thanks.

Eli Zaretskii <eliz>
Project Member
Tue 15 Oct 2013 09:25:52 PM UTC, comment #18:

Yep, patch in file #29381 works for me as well
(tested with both x32 and x64 toolchains of
MinGW-w64). Thanks!


Googling shows that lots of projects stumble upon
this issue. MinGW-w64 chose to be compatible with
the behavior of the MS API by default, see
http://thread.gmane.org/gmane.comp.gnu.mingw.w64.general/7132/focus=7133
and this different approach is indeed a problem when
MinGW-w64 is used as a drop in replacement for MinGW32.

They have documented the vsnprintf behavior here:
http://sourceforge.net/apps/trac/mingw-w64/wiki/printf%20and%20scanf%20family

Christian Boos <cboos>
Tue 15 Oct 2013 07:46:27 PM UTC, comment #17:

Sorry, wrong patch. Please use this one instead.

(file #29381)

Eli Zaretskii <eliz>
Project Member
Tue 15 Oct 2013 07:37:22 PM UTC, comment #16:

Thanks.

My debugging shows that the problem does not happen with the MinGW32 build, because vsnprintf is replaced by a conforming implementation in libmingwex.a, which is linked in by default. (So I guess mingw.org's distribution is not as bad as it sounds.)

Therefore, only MinGW64 builds need a fix. Please try the patch in the attachment; it is relative to stock Make 4.0.

(file #29380)

Eli Zaretskii <eliz>
Project Member
Tue 15 Oct 2013 07:30:37 AM UTC, comment #15:

As requested, 3 debugging sessions that recap the whole problem
with vsnprintf for MinGW-w64.

(file #29378)

Christian Boos <cboos>
Tue 15 Oct 2013 02:47:57 AM UTC, comment #14:

Please show me the debugging session where you saw the problem, with all the details. I need to see that to make sure we understand the problem completely. Just see how many different hypotheses and suggestions were offered in this discussion, and I'm sure you will understand why.

Thank you.

Eli Zaretskii <eliz>
Project Member
Mon 14 Oct 2013 10:27:24 PM UTC, comment #13:

I tested that patch (file #29377) with the mingw.org
toolchain (gcc 4.4.0) and make still works after that,
so I think it's safe to apply, provided this doesn't disturb
the cygwin build...

I also looked at mingw.org's mingw32/include/stdio.h and in fact
the __USE_MINGW_ANSI_STDIO flag doesn't select another vsnprintf
function, as opposed to what happens in MinGW32-w64's stdio.h.

As they write: "if you want the MSVCRT behaviour,
you must use the Microsoft uglified names."
(i.e. _vsnprintf, as a quick -Dvsnprintf=_vsnprintf confirmed).

Christian Boos <cboos>
Mon 14 Oct 2013 09:30:59 PM UTC, comment #12:

Yes, that would work, but the #define has to be added
much earlier, as even makeint.h includes stdio.h...

See 0004c-Ask-MinGW-w64-to-use-ANSI-style-vsnprintf.patch

(file #29377)

Christian Boos <cboos>
Mon 14 Oct 2013 09:20:55 PM UTC, comment #11:

Is it sufficient to add this line to output.c, like this:

?? Be sure to clean up any other local changes before testing. If that's good enough then it's much simpler than trying to change the compiler parameters to add a -D flag.

Paul D. Smith <psmith>
Project Administrator
Mon 14 Oct 2013 09:15:42 PM UTC, comment #10:

... and I just debugged the make built with the MinGW-w64
toolchain for x64 and without the -D__USE_MINGW_ANSI_STDIO=1
flag, the problem is indeed that vsnprintf returns -1 when the
fmtbuf is not large enough.

Also, I must have forgotten to revert my patch when I built
the 32-bits version earlier, as now I can also reproduce the
problem with also with make built with the MinGW-w64 x32
toolchain.

Bottom line: the mingw.org toolchain probably enables
the ANSI-like behavior by default, whereas the MinGW-w64
toolchains needs the -D__USE_MINGW_ANSI_STDIO=1 flag
for that.

Christian Boos <cboos>
Mon 14 Oct 2013 08:28:16 PM UTC, comment #9:

> (Btw, what is gnumake32.exe in your case, and how is it

different from gnumake64.exe?)

Sorry if that wasn't clear: gnumake32.exe was built with
the 32-bits toolchain from MinGW-w64, and gnumake64.exe
was built with the 64-bits toolchain from MinGW-w64.
Only the 64-bits build presents this issue, which is
also why you don't see it with the (32-bits, obviously)
build done with the mingw.org toolchain.

> So we need some other preprocessor symbols.


Right. What about simply _WIN64, as it doesn't seem
that mingw.org will support that anytime soon?
Moreover I just found out that in MinGW-w64 the support
for _vsnprintf_s function dates back nearly from the
beginning of the project, so no need for a version check.

Besides, I tried -D__USE_MINGW_ANSI_STDIO=1 and this was
also enough to fix the issue, indeed! So this is probably
the way to go.

Christian Boos <cboos>
Mon 14 Oct 2013 08:18:48 PM UTC, comment #8:

Eli, I think that most of the GCC-on-Windows world have moved over to MinGW-w64 (for both 32bit and 64bit) and I would encourage you to do the same as you'd be in-sync with the majority of your users that way. Of course having gnumake work well on both is best but if you must pick ..

Also, you should use -D__USE_MINGW_ANSI_STDIO=1

Ray Donnelly <raydonnelly>
Mon 14 Oct 2013 08:03:25 PM UTC, comment #7:

You are using MinGW64. I use MinGW32 from mingw.org, a different distribution with a different set of headers.

The tests you propose on __MINGW32_MAJOR_VERSION etc. will not distinguish between MinGW64 and MinGW32. So we need some other preprocessor symbols.

More importantly, I don't see the problems you report:

D:\gnu\make-4.0>gnumake -f no-Makefile
gnumake: no-Makefile: No such file or directory
gnumake: *** No rule to make target 'no-Makefile'. Stop

See: no problem. (Btw, what is gnumake32.exe in your case, and how is it different from gnumake64.exe?)

Can you please step with a debugger into the offending code, and actually show me what is failing there, and why?

Anyway, the only problem I know of with vsnprintf provided by the MS runtime is that it returns -1 when there's not enough space in the buffer, instead behaving like ANSI requires. If that is the problem (but I still want to see the code that fails due to this), it should be easy to fix with either (a) a trivial one-liner that handles a negative return value as a value larger than the buffer, or (b) by compiling Make with -D__USE_MINGW_ANSI_STDIO, which causes these functions to be replaced with ANSI-compatible versions supplied by MinGW.

Eli Zaretskii <eliz>
Project Member
Mon 14 Oct 2013 07:37:52 PM UTC, comment #6:

I found the _vsnprintf_s declaration in my MinGW installation:
And this gets included from <stdio.h>.
I suppose this must be a relatively recent addition,
as I installed that version today (major.minor is 3.11).

I added a version check to the patch, the numbers can be
refined to pinpoint the introduction of _vsnprintf_s, if
the approach is found to be valid and no better fix is
found.

Back to the issue itself. Here are a few quick ways to
reproduce it reliably.

First without a Makefile:

or with a minimal Makefile, which I inline here using --eval,
but the effect is the same with a Makefile containing that
single line:

Updated patch: 0004b-MinGW-also-needs-the-msc_vsnprintf-replacement-for-.patch

(file #29376)

Christian Boos <cboos>
Mon 14 Oct 2013 06:47:10 PM UTC, comment #5:

Moreover, MinGW does not provide _vsnprintf_s (or any of the *_s functions), so I don't see how did Make compile for you after the change. It fails for me.

Eli Zaretskii <eliz>
Project Member
Mon 14 Oct 2013 06:16:27 PM UTC, comment #4:

Please provide a minimal Makefile that can be used to reproduce the problem. Thanks.

I also don't understand why you needed to define va_copy for MinGW. AFAICS, MinGW does have a proper definition for it (MSVC indeed doesn't, AFAIK).

Eli Zaretskii <eliz>
Project Member
Mon 14 Oct 2013 05:41:22 PM UTC, comment #3:

Actually, it seems that this output problem also affects
MinGW builds, not just MSVC builds.

The patch 0004-MinGW-also-needs-the-msc_vsnprintf-replacement-for-v.patch
fixed it for me.

As I don't have Cygwin I couldn't verify if the WINDOWS32 define
is also defined there. If it is, the check must be changed to:

(file #29374)

Christian Boos <cboos>
Sat 12 Oct 2013 02:57:17 PM UTC, comment #2:

Thanks for the patches!

I had stumbled on a problem in which only the first command of a recipe would be printed. The second patch addresses that problem, as mentioned above.

Cheers,
Gerte Hoogewerf

Gerte Hoogewerf <ghoogewerf>
Fri 11 Oct 2013 11:47:38 AM UTC, comment #1:

The issue addressed by the second patch also manifests itself when using $(error) and $(warning), which don't print a message most of the time.

Mike Hommey <glandium>
Wed 09 Oct 2013 11:42:35 PM UTC, original submission:

Following the announcement, I did a checkout and attempted
to build make from the 4.0 tag, using the MSVC compilers
on Windows.

As I build preferentially from a bash shell, I tried
the "nmake -f NMakefile" way still described in the
README.W32.template file.

Unfortunately, the NMakefile.template was not complete.
This is fixed in 0001-Fix-MSVC-build-using-the-NMakefile.patch.

Second issue was detected at runtime, I had some crashes
happening in _vsnprintf. Looking at the code in output.c
it seemed that with the MSVC compiler _vsnprintf was not
supposed to be called as there's a msc_vsnprintf
replacement function. However the macro was called snprintf...
This is fixed in 0002-Fix-typo-preventing-the-msc_vsnprintf-function-to-be.patch.

Last fix, which is more a suggestion than a fix, consists
of increasing the default stack size, which proved to be
insufficient for me with a x64 build (the x86 version was
doing fine, so I suppose that the stack grows faster in the
x64 case).
See 0003-For-MSVC-build-augment-the-stack-size.patch.

With these patches (plus the fix I submitted separately in
#40226), make 4.0 seems to work like a charm on Windows
(tested with a big build started with '-j6 -Otarget' using
make.exes built with MSVC8, MSVC9, MSVC10 and MSVC11,
x64 builds).

I no longer have earlier versions of MSVC, so I don't know
if /STACK was OK there or not (I suppose it's OK).
Btw, some of the options used (/YX) are no longer valid,
others are deprecated (/GX). This results in annoying
warnings. Should I follow-up with a patch for removing them,
or is having these options still valued? (presumably they're
needed for older MSVC compilers).

Christian Boos <cboos>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #29381:  vsnprintf2.dif added by eliz (430B - application/vnd.ms-excel)
file #29380:  vsnprintf.dif added by eliz (346B - application/vnd.ms-excel - Patch for MinGW64)
file #29378:  gdb-trace-vsnprintf.txt added by cboos (14kB - text/plain)
file #29343:  0001-Fix-MSVC-build-using-the-NMakefile.patch added by cboos (1kB - application/octet-stream - hm, ignore the other 0001-Fix-test-etc. belonging to the other bug...)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by psmith (Posted a comment)
  • -unavailable- added by raydonnelly (Posted a comment)
  • -unavailable- added by eliz (Posted a comment)
  • -unavailable- added by ghoogewerf (Posted a comment)
  • -unavailable- added by glandium (Posted a comment)
  • -unavailable- added by cboos (Submitted the item)
  • -unavailable- added by cboos
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 14 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 23 Oct 2013 04:59:16 PM UTCelizStatusNone=>Fixed
      Open/ClosedOpen=>Closed
      Fixed ReleaseNone=>SCM
    Tue 15 Oct 2013 07:46:27 PM UTCelizAttached File-=>Added vsnprintf2.dif, #29381
    Tue 15 Oct 2013 07:37:22 PM UTCelizAttached File-=>Added vsnprintf.dif, #29380
    Tue 15 Oct 2013 07:30:37 AM UTCcboosAttached File-=>Added gdb-trace-vsnprintf.txt, #29378
    Mon 14 Oct 2013 09:30:59 PM UTCcboosAttached File-=>Added 0004c-Ask-MinGW-w64-to-use-ANSI-style-vsnprintf.patch, #29377
    Mon 14 Oct 2013 07:37:52 PM UTCcboosAttached File-=>Added 0004b-MinGW-also-needs-the-msc_vsnprintf-replacement-for-.patch, #29376
    Mon 14 Oct 2013 05:41:22 PM UTCcboosAttached File-=>Added 0004-MinGW-also-needs-the-msc_vsnprintf-replacement-for-v.patch, #29374
    Wed 09 Oct 2013 11:44:27 PM UTCcboosAttached File-=>Added 0001-Fix-MSVC-build-using-the-NMakefile.patch, #29343
    Wed 09 Oct 2013 11:42:35 PM UTCcboosAttached File-=>Added 0001-Fix-test-detecting-when-a-stringlist-must-grow-in-de.patch, #29340
      Attached File-=>Added 0002-Fix-typo-preventing-the-msc_vsnprintf-function-to-be.patch, #29341
      Attached File-=>Added 0003-For-MSVC-build-augment-the-stack-size.patch, #29342
      Carbon-Copy-=>Added cboos

    Back to the top


    Powered by Savane 3.1-cleanup