Past releases had a debug-only assertion that 25 was sufficient, but
then dynamically allocated enough storage for larger models as needed.
Commit 22a34809ee ("Move interpreter g3s_lrgb onto stack") added a
hard limit of 25, but did not detect attempts to exceed this. Custom
models that exceeded 25 would cause a stack buffer overwrite and likely
crash. Raise the limit to 64 and add sanity checking to refuse to
render models that exceed MAX_POINTS_PER_POLY.
Fixes: 22a34809ee ("Move interpreter g3s_lrgb onto stack")
Disable gcc's -Wformat-nonliteral for this one call since unchecked
format strings are (1) under the user's control, (2) unable to cause
problems, and (3) very difficult to fully support with the warning
enabled.
Reported-by: parkerlreed <https://github.com/dxx-rebirth/dxx-rebirth/issues/338>
For switch cases where existing comments or code flow logic obviously
intended to fall through, add a gcc-7 /*-fallthrough*/ comment to
silence warnings about this. Some cases which are less obvious are not
converted, so the code does not yet compile clean with
-Wimplicit-fallthrough.
Reported-by: parkerlreed <https://github.com/dxx-rebirth/dxx-rebirth/issues/338>
Compile time sanity checks are built on gcc's __builtin_constant_p.
This intrinsic is useful for optimization and for some simple forms of
compile-time checks (such as the infamous open(2) missing mode), but
upstream does not guarantee that it will work reliably for more
complicated checks. Starting in gcc-7.1, __builtin_constant_p returns
an incorrect result of true for the vm_vec_sub expression
`(a.x == b.x && a.y == b.y && a.z == b.z)`
even when the blamed sites clearly cannot prove that the inputs are
equal. The useful result would be to return true if, and only if, the
inputs were provably identical; inputs which might be identical at
runtime, or might not, would return false. Based on a bug filed with
gcc and the developer comments there, it appears many projects have
assumed this intrinsic is usable in this way, but the gcc developers do
not guarantee that it can be used this way. Additionally, they believe
affected projects are rare and were wrong to use this intrinsic, so they
have no plans to fix this regression.
For more details, see gcc bug #72785
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72785>. For some more
pointed commentary on this change in gcc, see Linus' kernel commit
474c90156c8dcc2fa815e6716cc9394d7930cb9c
<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=474c90156c8dcc2fa815e6716cc9394d7930cb9c>.
Update the SConf test to include the reproducer shown by Markus
Trippelsdorf in gcc bug #72785, comment #0. This reproducer compiles
cleanly on <gcc-7, causing no change in semantics on older compilers.
Affected versions of gcc-7 will miscompile this reproducer into a link
error, causing SConf to record that the compiler does not optimize
__builtin_constant_p. This is pessimistic, since even affected versions
of gcc-7 can handle some simple uses of __builtin_constant_p correctly.
This is a quick fix to get gcc-7 users working. Upstream seems
disinclined to revert to the more useful semantics of <gcc-7 or to
introduce an alternative intrinsic with more helpful semantics. As a
minor enhancement for Rebirth, it would be nice to probe the limits of
gcc-7's handling of __builtin_constant_p so that cases it handles
correctly could be enabled for gcc-7 users, while still blacklisting the
more complicated checks that gcc-7 miscompiles.
Thanks to Markus Trippelsdorf for providing a minimal reproducer to
detect the affected gcc versions.
Reported-by: parkerlreed <https://github.com/dxx-rebirth/dxx-rebirth/issues/337>
The dump logger probes for the end of the stack, then rounds down to the
nearest paragraph boundary to simplify the logic in the hexdump routine.
The termination condition in the hexdump code assumed that there would
exist an integer N such that (`start` + (16 * N) == `end`). Since `end`
is rounded to a multiple of 16, this held if and only if `start` is also
a multiple of 16. In practice, this tended to happen, but it was not
guaranteed by the code. If it ever failed to happen, then the hexdump
routine would not terminate and would instead perform an invalid read
beyond the edge of the stack.
Modify the hexdump routine to round `start` to a multiple of 16 so that
the termination condition works as intended. This has the useful side
effect that hex dumps now always start paragraph aligned. When the
stack was not paragraph aligned, this change will cause the hexdump to
show bytes below the stack pointer at the time of the fault. However,
the stack requirements of the handler itself ensure that these bytes
will be valid.
When Rebirth is configured to use OpenGL, it cannot start with an SDL
that lacks OpenGL support. Detect this mismatch at build time. Require
the user to resolve the conflict by disabling Rebirth OpenGL or by
enabling SDL OpenGL.
Hat labels reserve an extra character for the arrow, which partially
masked this error. When used buttons requires more characters than (1
+ used hats), the buffer had insufficient space and the label was
truncated.
Commit 829e95b dropped a test for game_mode_hoard when computing light.
Since hoard orbs are in a union which overlaps the score in mission
mode, this caused a bug where a player in mission mode with non-zero
score would pulse light like a player carrying orbs in hoard mode.
Restore the test for hoard mode.
Fixes: 829e95b6f8 ("Separate hoard/proximity tracking")
The Windows implementation of inet_ntop incorrectly omits the `const`
qualifier on the input address. This broke the Windows build, since
Rebirth passes a const-qualified input address, as permitted by POSIX.
Some Windows cross-compilers, such as mingw32, lack a definition of
inet_ntop entirely. For such environments, fall back to inet_ntoa and
disallow building with IPv6.
Rework the formatting and add a Windows-specific test for whether
inet_ntop is available. As inet_ntop is specified by POSIX, assume all
modern non-Windows platforms support it.
x86_64-w64-mingw32-g++ -Wformat handling misparses the std::size_t
format string, causing a spurious error.
common/main/valptridx.tcc: In function 'void untyped_index_mismatch_exception::prepare_report(const char*, unsigned int, const void*, long int, const void*, const void*, char (&)[229], std::size_t)':
common/main/valptridx.tcc:36:182: error: format '%u' expects argument of type 'unsigned int', but argument 7 has type 'std::size_t {aka long long unsigned int}' [-Werror=format=]
This occurs even though the processed text uses %I64u (which is correct
for a `long long unsigned int`), not %u as shown in the error message.
static void prepare_report(const char *const filename, const unsigned lineno, const void *const array_base, const long supplied_index, const void *const expected_pointer, const void *const actual_pointer, char (&buf)[report_buffer_size], const std::size_t array_size)
{
snprintf(buf, sizeof(buf), "%s:%u: " "pointer/index mismatch:" " base=%p size=%" "I64u" " index=%li expected=%p actual=%p", filename, lineno, array_base, array_size, supplied_index, expected_pointer, actual_pointer);
}
In practice, all such sizes will fit in `unsigned int` because they are
the dimension of an array of large structures. Switch all platforms to
use `unsigned long`, which works everywhere and is at least as big as
`unsigned int`. Using `unsigned long` produces the same size as
`std::size_t` on all platforms other than Win64, where `unsigned long`
is only 32 bits due to the strange LLP64 model Microsoft picked.
Commit b32298df5a ("Rewrite powerup cap
code to centralize logic") centralized powerup cap code in the
powerup_cap_state class.
Commit 901a554e96 ("New powerup management
code: Addeed functions and packet type to ...") removed all use of
powerup_cap_state, but left the dead implementation present.
Commit 479f5ed584 ("Fix 'format specifies
type 'unsigned short' but the argument has type 'unsigned char''
warning") switched the already dead (but still compiled) code from %hu
to %hhu to fix a warning on OS X. Although the commit was written by
Chris, it was my suggestion to use %hhu. I neglected to test Windows
before suggesting it, so the change went in even though Windows does not
accept %hhu; this broke the Windows build. Fortunately, the code had
been dead for 11 months when the change was made, so the fix for Windows
is to remove the long dead code.
SConstruct prefers to refer to linker flags for the main executable as
LINKFLAGS, not LDFLAGS. Rename the internal storage to eliminate a
special case in accessing it. Continue to use the environment variable
$LDFLAGS as an initial value, since many tools expect to pass linker
flags through $LDFLAGS.
