Fix clang + valgrind conditional jump depending on uninitialised value in CGUIManager::HandleEvent false positive warning following rP7259
ClosedPublic
Actions

Authored by elexis on Aug 24 2019, 8:10 PM.

Details

Reviewers: None
Commits: rP22777: Fix valgrind conditional jump depending on uninitialised value warning in…

Summary

In rP7259 there was a local variable handled without value initialization resulting in undefined behavior.

==32499== Conditional jump or move depends on uninitialised value(s)
==32499==    at 0x5A250F: CGUIManager::HandleEvent(SDL_Event_ const*) (GUIManager.cpp:330)
==32499==    by 0x5A1F77: gui_handler(SDL_Event_ const*) (GUIManager.cpp:48)
==32499==    by 0x60CF49: in_dispatch_event(SDL_Event_ const*) (input.cpp:62)
==32499==    by 0x187708: PumpEvents (main.cpp:227)
==32499==    by 0x187708: Frame (main.cpp:367)
==32499==    by 0x187708: RunGameOrAtlas(int, char const**) (main.cpp:638)
==32499==    by 0x1851B4: main (main.cpp:684)
==32499==  Uninitialised value was created by a stack allocation
==32499==    at 0x5A1FC0: CGUIManager::HandleEvent(SDL_Event_ const*) (GUIManager.cpp:315)
==32499== 
==32499== Conditional jump or move depends on uninitialised value(s)
==32499==    at 0x60CF4D: in_dispatch_event(SDL_Event_ const*) (input.cpp:64)
==32499==    by 0x187708: PumpEvents (main.cpp:227)
==32499==    by 0x187708: Frame (main.cpp:367)
==32499==    by 0x187708: RunGameOrAtlas(int, char const**) (main.cpp:638)
==32499==    by 0x1851B4: main (main.cpp:684)
==32499==  Uninitialised value was created by a stack allocation
==32499==    at 0x5A1FC0: CGUIManager::HandleEvent(SDL_Event_ const*) (GUIManager.cpp:315)
==32499== 
==32499== Conditional jump or move depends on uninitialised value(s)
==32499==    at 0x60CF52: in_dispatch_event(SDL_Event_ const*) (input.cpp:67)
==32499==    by 0x187708: PumpEvents (main.cpp:227)
==32499==    by 0x187708: Frame (main.cpp:367)
==32499==    by 0x187708: RunGameOrAtlas(int, char const**) (main.cpp:638)
==32499==    by 0x1851B4: main (main.cpp:684)
==32499==  Uninitialised value was created by a stack allocation
==32499==    at 0x5A1FC0: CGUIManager::HandleEvent(SDL_Event_ const*) (GUIManager.cpp:315)

Test Plan

This is the very first thing you see if having compiled with clang 8.0.1, but you won't see it with gcc 9.1.0.

Modify this file so that the following command compiles this translation unit with clang:
CC="clang" CXX="clang++" make -j3
valgrind --leak-check=full --track-origins=yes ./pyrogenesis

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 9057
Build 14841: Vulcan Build	Jenkins
Build 14840: arc lint + arc unit

Event Timeline

elexis created this revision.Aug 24 2019, 8:10 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/480/display/redirect

Harbormaster completed remote builds in B9057: Diff 9488.Aug 24 2019, 8:15 PM

elexis retitled this revision from Fix conditional jump depending on uninitialised value in CGUIManager::HandleEvent following rP7259 to Fix clang + valgrind conditional jump depending on uninitialised value in CGUIManager::HandleEvent false positive warning following rP7259.Aug 24 2019, 9:03 PM

Notice it complains about the CallFunction condition, not the (handled) line after that.

As pointed out by Vladislav,
according to the code it should be a false positive, because the return value is always determined by either the failure of calling the JS function, or by the failure of convert the result to boolean - from JS.
The failure or success of the JS function does not depend on teh return value, since the variable is not used by that call yet (only js return value).

There are others who reported about false positives for valgrind:
https://github.com/Z3Prover/z3/issues/972

Discussion with Vladislav on http://irclogs.wildfiregames.com/2019-08/2019-08-24-QuakeNet-%230ad-dev.log

19:53 < elexis> Reviewed By:?
19:56 < Vladislav_> Ok.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 25 2019, 12:25 AM

Closed by commit rP22777: Fix valgrind conditional jump depending on uninitialised value warning in… (authored by elexis). · Explain Why

This revision was automatically updated to reflect the committed changes.

elexis added a commit: rP22777: Fix valgrind conditional jump depending on uninitialised value warning in….

elexis mentioned this in P170 Conditional jump or move depends on uninitialised value IGUIObject::SendEvent IXmppClient::create JS_CallFunctionValue.Sep 9 2019, 10:20 PM

Revision Contents
Changeset List

Path

Size

source/

gui/

GUIManager.cpp

2 lines

Diff 9488

View Options

Fix clang + valgrind conditional jump depending on uninitialised value in CGUIManager::HandleEvent false positive warning following rP7259ClosedPublicActions