Page MenuHomeWildfire Games

Do not rely on undefined behavior in an unsupported part of the glooxwrapper.
ClosedPublic

Authored by Itms on May 4 2017, 1:37 AM.

Details

Summary

Pointed out by a clang warning, original patch by leper.

Test Plan

Build and test by building the glooxwrapper (update-workspaces.bat --build-shared-glooxwrapper).

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4845
Build 8396: Vulcan BuildJenkins
Build 8395: arc lint + arc unit

Event Timeline

leper created this revision.May 4 2017, 1:37 AM
Vulcan added a subscriber: Vulcan.May 4 2017, 2:31 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/993/ for more details.

Itms requested changes to this revision.May 26 2017, 7:23 PM
Itms added a reviewer: Itms.
Itms added a subscriber: Itms.

This does not build, both on Windows and Linux: linking fails with undefined reference to debug_break() and debug_OnAssertionFailure().

Don't forget you need to call update-workspaces.sh --build-shared-glooxwrapper to actually run a build of it.

This revision now requires changes to proceed.May 26 2017, 7:23 PM
leper updated this revision to Diff 4355.Nov 25 2017, 3:51 AM

Actually fail. Not really more tested than the previous iteration, though I have had this iteration in my local copy for a while.

leper updated this revision to Diff 4356.Nov 25 2017, 3:52 AM

Now with the correct commit.

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...

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

Updating workspaces...
Build (release)...
../../../source/lobby/glooxwrapper/glooxwrapper.cpp:222:48: warning: unused parameter ‘from’ [-Wunused-parameter]
  virtual void handleDataForm(const gloox::JID& from, const gloox::DataForm& UNUSED(form))
                                                ^
../../../source/lobby/glooxwrapper/glooxwrapper.cpp:228:43: warning: unused parameter ‘from’ [-Wunused-parameter]
  virtual void handleOOB(const gloox::JID& from, const gloox::OOB& UNUSED(oob))
                                           ^
Build (debug)...
../../../source/lobby/glooxwrapper/glooxwrapper.cpp:222:48: warning: unused parameter ‘from’ [-Wunused-parameter]
  virtual void handleDataForm(const gloox::JID& from, const gloox::DataForm& UNUSED(form))
                                                ^
../../../source/lobby/glooxwrapper/glooxwrapper.cpp:228:43: warning: unused parameter ‘from’ [-Wunused-parameter]
  virtual void handleOOB(const gloox::JID& from, const gloox::OOB& UNUSED(oob))
                                           ^
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...
Itms accepted this revision.Nov 25 2017, 2:17 PM
Itms removed reviewers: Restricted Owners Package, Restricted Owners Package.

Tested and works, I accept it but you should add UNUSED to the from parameters as reported by Vulcan.

This revision is now accepted and ready to land.Nov 25 2017, 2:17 PM
leper updated this revision to Diff 4431.Nov 29 2017, 3:27 AM

Now with UNUSED().

In D422#42077, @Itms wrote:

Tested and works [...]

I assume you built passed the parameter for building the shared glooxwrapper?

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...
leper abandoned this revision.Dec 19 2017, 6:41 PM

Feel free to commit this in case you think it helps with not invoking undefined behaviour.

Itms added a comment.Feb 6 2018, 4:08 PM

I was looking at this again and I don't know the glooxwrapper well enough to determine why those two functions are "not supported" (I don't see the difference in the code between them and, for instance, handleRegistrationResult). Once I understand this maybe I will see whether there is a specific reason for failing here instead of just doing nothing in the function (like we do in all the other void functions that are marked as "Not supported"). ?

Itms updated this revision to Diff 5690.Feb 7 2018, 8:56 AM

Don't explicitly fail, but do nothing (as we do for the rest of the unsupported API) instead of relying on
undefined behavior.

This revision is now accepted and ready to land.Feb 7 2018, 8:56 AM
Itms commandeered this revision.Feb 7 2018, 8:57 AM
Itms edited reviewers, added: leper; removed: Itms.
This revision now requires review to proceed.Feb 7 2018, 8:57 AM
Itms retitled this revision from Explicitly fail in case something tries to use this, instead of invoking undefined behaviour. to Do not rely on undefined behavior in an unsupported part of the glooxwrapper..Feb 7 2018, 8:59 AM
Itms edited the summary of this revision. (Show Details)
Itms edited the test plan for this revision. (Show Details)
Vulcan added a comment.Feb 7 2018, 8:59 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Itms updated this revision to Diff 5691.Feb 7 2018, 9:10 AM

Without the unneeded includes.

Vulcan added a comment.Feb 7 2018, 9:14 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Feb 7 2018, 9:16 AM
Executing section Default...
Executing section Source...
Executing section JS...

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/1/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/2/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/differential/3/display/redirect

Itms edited reviewers, added: Windows Developers; removed: leper.Feb 11 2018, 5:17 PM

Would one of you guys have time to test this on Windows?

Stan added a subscriber: Stan.Feb 14 2018, 8:11 PM

Going to test it now :)

Stan added a comment.Feb 14 2018, 8:25 PM

Replaced

if not exist ..\workspaces\vc2015\SKIP_PREMAKE_HERE premake5\bin\release\premake5 --outpath="../workspaces/vc2015" --use-shared-glooxwrapper %* vs2015

by

if not exist ..\workspaces\vc2015\SKIP_PREMAKE_HERE premake5\bin\release\premake5 --outpath="../workspaces/vc2015" --build-shared-glooxwrapper %* vs2015

Built , no errors, Then logged into the lobby successfully. Anything else ?

Stan accepted this revision as: Stan.Feb 14 2018, 11:20 PM
This revision is now accepted and ready to land.Feb 14 2018, 11:20 PM
This revision was automatically updated to reflect the committed changes.