Use %zu for printf-style formatting of size_t values. #19339

mutability · 2016-11-19T13:04:36Z

This is a semi-automated replacement of %d/%i formats with %zu
when the parameter is of type size_t. It was driven by parsing the
warnings generated by PRINTF_CHECKS.

Also fixes one case where a size_t was passed as a width specifier
(this has to be an int)

%zu is specified in C++11 (via C99) so hopefully we are OK with
compatibility there. The alternatives are very ugly (macros embedded
in format strings, or casts everywhere)

This is a semi-automated replacement of %d/%i formats with %zu when the parameter is of type size_t. It was driven by parsing the warnings generated by PRINTF_CHECKS. Also fixes one case where a size_t was passed as a width specifier (this has to be an int) %zu is specified in C++11 (via C99) so hopefully we are OK with compatibility there. The alternatives are very ugly (macros embedded in format strings, or casts everywhere)

illi-kun · 2016-11-20T14:01:04Z

According to stackoverflow thread, VS2013 doesn't support %zu. I'm sure this is an issue but I think it's better to state it here.

mutability · 2016-11-20T14:19:48Z

VS2013 also doesn't support C++11, does it? My research seemed to indicate that the more recent visual studio versions that supported C++11, also supported %zu.

mutability · 2016-11-20T14:26:40Z

http://stackoverflow.com/questions/15610053/correct-printf-format-specifier-for-size-t-zu-or-iu suggests that VS2015 does support %zu, VS2013 may or may not it's unclear.

What are we targeting?

(VS2013 apparently doesn't support constexpr either..)

illi-kun · 2016-11-20T16:02:20Z

Okay, I've checked compiling.md and it seems that we targeted on support of VS2015:

Visual Studio 2015 is required to build Cataclysm.

So you PR is acceptable.

Totentag · 2016-11-20T18:54:22Z

Unintended bugs coming from this.

macrosblackd · 2016-11-20T18:58:47Z

This breaks the Windows release by causing a crash when opening the new inventory UI.

mutability · 2016-11-20T19:07:31Z

What is the Windows release built with?

DanmakuDan · 2016-11-20T19:28:18Z

MXE cross compiles with MinGW, most likely 4.8.2 from #13652.

mutability · 2016-11-20T19:29:09Z

Looks like we might need -D__USE_MINGW_ANSI_STDIO=1 to tell it to use the standards-conforming *printf not the Microsoft one.

https://stackoverflow.com/questions/25670522/c-program-shows-zu-after-conversion-to-windows

I have no way of testing this.

DanmakuDan · 2016-11-20T20:31:19Z

Maybe @narc0tiq can check if the flag is applicable to the build. This issue isn't present on MSYS2/mingw gcc 5.2.0.

illi-kun · 2016-11-20T20:33:05Z

Yeah, I tested these charges with Code::Blocks default compiler in Windows and it woorks fine.

illi-kun · 2016-11-20T21:32:37Z

It seems that this PR needs to be reverted, sorry for this. Is it ok to just push the 'revert' button or we have some more gentle and safe way to do that?

codemime · 2016-11-20T21:32:48Z

Shouldn't we revert the PR until the solution is applied? BTW the link that @mutability gave suggests a [quick]fix:

all you need to do to get all of this standard behavior is either #define __USE_MINGW_ANSI_STDIO 1 before any includes if you use any of the printf functions or ...

codemime · 2016-11-20T21:33:17Z

@illi-kun At the same time.

codemime · 2016-11-20T21:46:46Z

sorry for this.

That's all right. The side effect wasn't easily predictable.

Is it ok to just push the 'revert' button or we have some more gentle and safe way to do that?

Pushing it creates a reverting pull request. I believe that's perfectly safe.

mutability · 2016-11-20T22:06:19Z

So what's the plan for actually fixing the build issue? I can't test the change I suggested, is there an active developer with the same build environment (same as the CI environment) who can test it?

(if there isn't, that's probably a sign that the CI environment needs to change)

narc0tiq · 2016-11-21T08:39:07Z

I can theoretically run builds of specific commits right in Jenkins itself, but that'd show up as a new build and would probably get grabbed up by the auto-updater, so let's avoid that.

At the same time, I can also just manually run the same commands in its shell and capture the output. If we're suggesting just that extra #define (as a CXXFLAGS addon), I'll have some binaries for you in a short while.

narc0tiq · 2016-11-21T09:18:11Z

Okay, here's the wincurses build log and binary taken by checking out this PR (specifically, commit 7dcb672) and adding CXXFLAGS='-D__USE_MINGW_ANSI_STDIO=1' to the build invocation[1].

In my very brief testing, the build is quite unstable; simple things like monsters fighting each other, or eating often (but not always) lead to crashing. This is likely to be the same issue as reported in #19372.

[1] Specifically, the invocation was CXXFLAGS='-D__USE_MINGW_ANSI_STDIO=1' BUILD_NUMBER=pr19339 Platform=Windows Graphics=Curses /www/virt/dev.narc.ro/htdocs/cataclysm/mainline-build.zsh > /www/virt/dev.narc.ro/htdocs/cataclysm/cataclysmdda-pr19339-build.log 2>&1, which should be identical with a Jenkins invocation except for the additional compiler flag.

mutability · 2016-11-21T14:12:21Z

OK, so if that didn't work, then options are (in rough order of my preference):

someone with an environment equivalent to the CI environment (so far that seems to mean @narc0tiq only) does some experimentation and works out how to make it compile with a C++11 compatible printf
update the CI environment to a version that supports C++11 properly (e.g. one of the other Windows build methods above which appears to work OK)
embed our own printf implementation, e.g. https://github.com/fmtlib/fmt, either everywhere or just on Windows builds. NB: this would also fix the existing positional-args problem too I think.
use a macro: ("hey I counted %" PRZU " things", things.size())
forbid use of size_t when formatting: ("hey I counted %d things", (int)things.size())

Doing nothing here is just delaying the inevitable as there are surely platforms where passing a size_t to %d is a crash bug.

codemime · 2016-11-21T14:29:40Z

The first one appears to be the only viable option.

mutability · 2016-11-21T15:20:20Z

Here's a minimalish test; if you can get that compiling in the CI environment such that it says "vector has 1 elements" when run, then it's probably working.

#include <cstdio>
#include <vector>

int main(int argc, char *argv[])
{
    std::vector<int> v;
    v.push_back(1234);
    printf("vector has %zu elements\n", v.size());
    return 0;
}

narc0tiq · 2016-11-21T16:55:16Z

Okay. It just worked. Here's the binary.

My command was ccache /home/narc/mxe/usr/bin/i686-w64-mingw32.static-g++ -ffast-math -Os -Wall -Wextra -fsigned-char -std=c++11 -MMD -c test.cpp -o test.obj && ccache /home/narc/mxe/usr/bin/i686-w64-mingw32.static-g++ -o test.exe test.obj -static (basically, a stripped-down version of the compile commands for the cata binary).

Notably, I left out the -D__USE_MINGW_ANSI_STDIO=1 -- on a previous pass, I had it in, and it also worked.

mutability · 2016-11-21T17:27:49Z

Hm, weird, why is it failing on the full build then..

mutability · 2016-11-21T17:34:03Z

Ok, so it looks like everything goes through vstring_format in output.cpp which has some _MSC_VER-specific magic that invokes _vscprintf_p and _vsprintf_p. I guess that is bypassing the mingw implementation.

The other non-microsoft path uses vsnprintf directly. I wonder if it is simple as dropping the #ifdef _MSC_VER path, can you try turning that ifdef off and using the vsnprintf version?

mutability · 2016-11-23T13:20:51Z

I have no real idea about this, the old MXE produces strange results and the new MXE works fine here.

narc0tiq · 2016-11-24T09:39:50Z

So I'm working on a new Jenkins and this is funny (but not promising): http://gorgon.narc.ro:8080/job/Cataclysm-Matrix/Graphics=Curses,Platform=Windows_x64/1/console

In file included from /home/narc/src/mxe/usr/lib/gcc/x86_64-w64-mingw32.static/4.9.4/include/c++/map:61:0,
                 from src/json.h:10,
                 from src/enums.h:8,
                 from src/visitable.h:9,
                 from src/character.h:4,
                 from src/player.h:4,
                 from src/bionics.cpp:1:
/home/narc/src/mxe/usr/lib/gcc/x86_64-w64-mingw32.static/4.9.4/include/c++/bits/stl_map.h: In member function 'bool player::activate_bionic(int, bool)':
/home/narc/src/mxe/usr/lib/gcc/x86_64-w64-mingw32.static/4.9.4/include/c++/bits/stl_map.h:212:56: internal compiler error: in expand_expr_addr_expr_1, at expr.c:7672
       { _M_t._M_insert_unique(__l.begin(), __l.end()); }
                                                        ^
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
Makefile:650: recipe for target 'objwin/bionics.o' failed
make: *** [objwin/bionics.o] Error 1
make: *** Waiting for unfinished jobs....
Build step 'Execute shell' marked build as failure
Recording fingerprints
Finished: FAILURE

Hooray for internal compiler errors.

mutability · 2016-11-24T12:31:33Z

Been a long time now since I saw an ICE..

I did only try a 32-bit build. Let me try a 64-bit build.

mutability · 2016-11-24T12:35:20Z

I get the same ICE with a 64-bit build. Doesn't happen with a 32-bit (i686) build.

mutability · 2016-11-24T12:41:27Z

Actually now that I look at the error, it is a different ICE. Joy.

In file included from /home/oliver/git/mxe/usr/lib/gcc/x86_64-w64-mingw32.static/4.9.4/include/c++/set:61:0,
                 from src/json.h:11,
                 from src/enums.h:8,
                 from src/sounds.h:4,
                 from src/sounds.cpp:1:
/home/oliver/git/mxe/usr/lib/gcc/x86_64-w64-mingw32.static/4.9.4/include/c++/bits/stl_set.h: In function 'void sfx::do_footstep()':
/home/oliver/git/mxe/usr/lib/gcc/x86_64-w64-mingw32.static/4.9.4/include/c++/bits/stl_set.h:225:56: internal compiler error: in expand_expr_addr_expr_1, at expr.c:7672
       { _M_t._M_insert_unique(__l.begin(), __l.end()); }
                                                        ^

mutability · 2016-11-24T12:47:17Z

Getting all sorts of ICEs at different locations depending on the exact compile options (but always in expand_expr_addr_expr_1, at expr.c:7672).

I do notice this in the Makefile:

    # MXE ICE Workaround
    ifeq (${CXXVERSION}, 4.9.3)
      OPTLEVEL = -O3
    else
      OPTLEVEL = -Os
    endif

perhaps that problem persists in 4.9.4

narc0tiq · 2016-11-24T12:51:33Z

Ah, hey, that could be exactly it! I'll experiment and report back.

mutability · 2016-11-24T12:57:23Z

Yeah, it seems to work OK here if I set -O3. Beware that the current makefile detection stuff is broken (it looks at the output of "gcc" not "$(CROSS)gcc"). I'll put together a patch for that.

…-mingw32.static As seen in CleverRaven#19339

narc0tiq · 2016-11-24T13:09:01Z

I'm getting an odd issue where the x86_64-w64-mingw32.static-g++ is apparently making i386 object files, leading to /home/narc/src/mxe/usr/bin/x86_64-w64-mingw32.static-ld: i386 architecture of input file 'objwin/mod_test.o' is incompatible with i386:x86-64 output. This is really special indeed. Maybe my ccache is ccaching the wrong thing.

narc0tiq · 2016-11-24T13:38:52Z

No, it's not ccache (it shouldn't have been, so I'm not surprised, but bleh). I need to get back to work; I'll come back to this later.

patternoia · 2016-11-24T19:31:55Z

As a bit of offtopic: @narc0tiq since you are deploying a new build server (or upgrading existing), could you please install clang/llvm v3.8.1. This will help with linux to mac cross compilation later.

You could very simply install it by cloning osxcross and running CLANG_VERSION=3.8.1 INSTALLPREFIX=/any/path/here ./build_clang.sh. By specifying the INSTALLPREFIX you then ensure you will not damage the current clang installation.
Let me know what you think about this.

narc0tiq · 2016-11-24T21:11:39Z

Hell, I can do that to the existing build server right now - I have packages available, only need to install them. Indeed, on the new machine, I went straight to clang-3.8 because why not?

…

On Thu, Nov 24, 2016, 21:31 patternoia ***@***.***> wrote: As a bit of offtopic: @narc0tiq <https://github.com/narc0tiq> since you are deploying a new build server (or upgrading existing), could you please install clang/llvm *v3.8.1*. This will help with linux to mac cross compilation later. You could very simply install it by cloning osxcross <https://github.com/tpoechtrager/osxcross> and running CLANG_VERSION=3.8.1 INSTALLPREFIX=/any/path/here ./build_clang.sh. By specifying the INSTALLPREFIX you then ensure you will not damage the current clang installation. Let me know what you think about this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19339 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkG1dvjAdq79V7RfmBN_ifFRJdDngvfks5rBeYugaJpZM4K3P7r> .

narc0tiq · 2016-11-25T09:53:00Z

So, I found out that make clean doesn't actually clean the tests/objwin. Maybe should fix that sometime.

narc0tiq · 2016-11-25T09:54:07Z

@patternoia I seem to have v3.8.0 on the new machine (but I haven't plugged in the clang PPA, which may resolve it by way of package (much preferable)). Is the .1 critical?

patternoia · 2016-11-25T10:21:45Z

Should not be

This is a semi-automated replacement of %d/%i formats with %zu when the parameter is of type size_t. It was driven by parsing the warnings generated by PRINTF_CHECKS. Also fixes one case where a size_t was passed as a width specifier (this has to be an int) Adds a testcase that tests that %zu and positional args work OK. This requires C++ / C99 printf support. The current CI environment does not provide this for Windows builds, so this needs to be merged _after_ the CI environment has been upgraded. (previously: CleverRaven#19339)

kevingranade · 2016-11-30T02:28:45Z

Just a project management note, leaving this reverted and trying again later would be preferable to an invasive workaround, this is a fairly obscure warning that has very little chance of causing actual problems, and presumably the underlying projects will have fixes applied in time.

mutability · 2016-11-30T03:01:33Z

What invasive workaround do you mean?

Fixing this particular warning is a necessary step towards fixing all the printf warnings, at which point we can turn on enforcement of those warnings in release builds (or as a separate check step) and avoid future bugs involving a mismatch between format strings and arguments.

narc0tiq · 2016-11-30T16:57:02Z

invasive workaround

I'm not sure it's that invasive, as it's just trading one set of optimizations for another (and it's fairly narrowly targeted at mingw64's x86_64 build target only). We can always narrow it further if someone needs it, but it's more likely mingw64 will update their gcc first... in a couple more years.

mutability · 2016-11-30T17:04:01Z

The -O3 thing is just keeping an existing workaround intact with the new version of gcc, isn't it?

narc0tiq · 2016-11-30T17:13:29Z

Yeah, but with #19440 it's targeted on mingw64 entirely -- which it probably needs, at least for now. Eventually, it'll be unnecessary and we can remove it altogether, I hope.

mutability · 2016-11-30T18:17:03Z

Are we building with anything other than gcc 4.9.3 / 4.9.4 for x86_64-w64-mingw32.static? I thought the answer to that was "no"

mutability · 2016-11-30T18:18:22Z

It would be useful if Kevin can clarify what he means too because I don't understand what the issue is - the ICE has nothing to do with this PR and you need it anyway?

kevingranade · 2016-12-03T23:02:03Z

I was referring to these options: 1. embed our own printf implementation, e.g. https://github.com/fmtlib/fmt, either everywhere or just on Windows builds. NB: this would also fix the existing positional-args problem too I think. 2. use a macro: ("hey I counted %" PRZU " things", things.size()) 3. forbid use of size_t when formatting: ("hey I counted %d things", (int)things.size()) If it's a targeted fix in the build system my only concern is it being a time sink, but that's up to the people involved to decide.

mutability · 2016-12-03T23:04:30Z

Yeah, agreed that none of those solutions are particularly good; fixing the CI environment is the way forward.

illi-kun self-assigned this Nov 20, 2016

illi-kun merged commit 779a385 into CleverRaven:master Nov 20, 2016

mutability deleted the printf-sizet branch November 20, 2016 16:45

macrosblackd mentioned this pull request Nov 20, 2016

Trying to (E)at something causes crash on 0.C-19820-g779a385 #19372

Closed

illi-kun mentioned this pull request Nov 20, 2016

Revert "Use %zu for printf-style formatting of size_t values." #19377

Merged

mutability added a commit to mutability/Cataclysm-DDA that referenced this pull request Nov 24, 2016

Update MXE ICE workaround to blacklist anything targetting x86_64-w64…

6c5cd05

…-mingw32.static As seen in CleverRaven#19339

mutability mentioned this pull request Nov 24, 2016

Update MXE ICE workaround to blacklist anything targetting x86_64-w64-mingw32.static #19440

Merged

mutability mentioned this pull request Nov 27, 2016

Use %zu for printf-style formatting of size_t values. #19488

Merged

narc0tiq mentioned this pull request Jan 1, 2017

0.C-20368-g28fa326 / 0.C-6093 examine pile/inventory with certain items -> crash #19894

Closed

kevingranade mentioned this pull request Jun 20, 2017

GCC 5.4.0 doesn't like src/iexamine.cpp #21285

Closed

Use %zu for printf-style formatting of size_t values. #19339

Use %zu for printf-style formatting of size_t values. #19339

Conversation

mutability commented Nov 19, 2016

illi-kun commented Nov 20, 2016

mutability commented Nov 20, 2016

mutability commented Nov 20, 2016

illi-kun commented Nov 20, 2016

Totentag commented Nov 20, 2016

macrosblackd commented Nov 20, 2016

mutability commented Nov 20, 2016

DanmakuDan commented Nov 20, 2016

mutability commented Nov 20, 2016 • edited

DanmakuDan commented Nov 20, 2016

illi-kun commented Nov 20, 2016

illi-kun commented Nov 20, 2016

codemime commented Nov 20, 2016 • edited

codemime commented Nov 20, 2016

codemime commented Nov 20, 2016

mutability commented Nov 20, 2016 • edited

narc0tiq commented Nov 21, 2016

narc0tiq commented Nov 21, 2016

mutability commented Nov 21, 2016

codemime commented Nov 21, 2016

mutability commented Nov 21, 2016

narc0tiq commented Nov 21, 2016 • edited

mutability commented Nov 21, 2016

mutability commented Nov 21, 2016

mutability commented Nov 23, 2016

narc0tiq commented Nov 24, 2016

mutability commented Nov 24, 2016

mutability commented Nov 24, 2016

mutability commented Nov 24, 2016

mutability commented Nov 24, 2016

narc0tiq commented Nov 24, 2016

mutability commented Nov 24, 2016 • edited

narc0tiq commented Nov 24, 2016

narc0tiq commented Nov 24, 2016

patternoia commented Nov 24, 2016

narc0tiq commented Nov 24, 2016 via email

narc0tiq commented Nov 25, 2016

narc0tiq commented Nov 25, 2016

patternoia commented Nov 25, 2016

kevingranade commented Nov 30, 2016 via email

mutability commented Nov 30, 2016

narc0tiq commented Nov 30, 2016

mutability commented Nov 30, 2016 via email

narc0tiq commented Nov 30, 2016

mutability commented Nov 30, 2016

mutability commented Nov 30, 2016

kevingranade commented Dec 3, 2016 via email

mutability commented Dec 3, 2016

mutability commented Nov 20, 2016 •

edited

codemime commented Nov 20, 2016 •

edited

mutability commented Nov 20, 2016 •

edited

narc0tiq commented Nov 21, 2016 •

edited

mutability commented Nov 24, 2016 •

edited