[Echelon9]

I had a chance to run the latest github hosted code through it's paces. I'm very impressed.

Whilst I wasn't able to test every single function or feature exposed through the debug_console, there was no Clang warnings or AddressSanitizer errors reported that are unique to z64's new code.

To add, there are some very handy functions which are being re-enabled via the debug_console. I would strongly recommend getting this feature back into trunk as soon as possible.

[z64555]

Alright, that should be the last of it.

I've rebased and repaired compiling of the debug_console branch so that its ready to go into the SVN trunk. For those of you who are unaware, I had to purge my fork of the fs2open repo and then recreate it. Couple this with a purge of my failed git-svn local repo, I had to go through a stack of .diff's and .patches to try and find one that was the most recent and up to date.

Please let me know if any issues arise.

[Edit]I sent in a pull request on github for some more practice using their interface.

[Yarn]

The gr_* functions that originally took booleans for resizing (such as gr_string() and gr_rect()) now take one of the GR_RESIZE_* defines in 2d.h. This change was necessary to support nonstretching menus.

However, your version of console.cpp still has calls that pass booleans to these fuctions. You should fix them to conform to the new system. (Just change "false" to "GR_RESIZE_NONE", and change "true" to "GR_RESIZE_MENU".)

[z64555]

Alright, committed and pushed to fork. AFAIK, none of the DCF's use any of gr_* functions. Those that do on-screen display stuff toggle a flag somewhere, and game_frame() or w/e takes care of it.

As I've said over IRC several times, I've been planning on making a gauge where DCF's will output to, instead of relying on various hacks here and there that do the draw calls themselves. I haven't gotten around to implementing this just yet, and would probably be better done on a new branch.

[m!m]

Or you could implement a scripting hook for it and let a modder handle it

[Echelon9]

Hi z64,

I'm still having some trouble with your Github branch, are you sure you don't have local files that you are yet to 'git add .', 'git commit', then 'git push origin debug_console' on?

For instance, I see you've added the following files by reference to the MSVC_2011 project file. They don't appear in the relevant folder however.

Code: [Select]

+    <ClCompile Include="..\..\code\debugconsole\consolecmds.cpp" />
+    <ClCompile Include="..\..\code\debugconsole\consoleparse.cpp" />
...
+    <ClInclude Include="..\..\code\debugconsole\console.h" />
+    <ClInclude Include="..\..\code\debugconsole\consoleparse.h" />
https://github.com/z64555/fs2open.github.com/tree/debug_console/code/debugconsole

[z64555]

 

Somehow those files got ignored at some point.

New commit Pushed to fork.

[Echelon9]

Thanks for the latest commit. You should now see my review comments on Github.

Key comments:

• A few remaining locations where non-POD types passed to variadic functions. Suggest adding the *.c_str() to each.
• There's an introduction of a C++11 feature, pop_back() on the std_string type. This raising of the minimum supported compiler might be inadvertent, as there is broad use of the old pop_back() on the deque container in that file. Suggest approach is discussed here in the forum thread.
• I experienced issues that required me to "#include "debugconsole/console.h" at the top of code/windows_stubs/stubs.cpp on OS X.

Once settled, I will add the new files to the Xcode project file. Updated: Attached here if you'd like to include in your debug_console Github branch.

[attachment deleted by an evil time traveler]

[z64555]

A few remaining locations where non-POD types passed to variadic functions. Suggest adding the *.c_str() to each.

  This issue keeps resurfacing. Thanks for spotting them.

I experienced issues that required me to "#include "debugconsole/console.h" at the top of code/windows_stubs/stubs.cpp on OS X.

There's a DCF_BOOL macro invoked at line 405, this is likely the cause. Since code/windows_stubs/ is not included in the MSVS project filters, I did not catch that. Thanks.

There's an introduction of a C++11 feature, pop_back() on the std_string type. This raising of the minimum supported compiler might be inadvertent, as there is broad use of the old pop_back() on the deque container in that file. Suggest approach is discussed here in the forum thread.

Which is strange, because according to both cplusplus.com and cppreference.com, std::deque.pop_back() has been in the stdlib far before C++11 rolled around. If this continues to be an issue, then switching to an SCP_list shouldn't be an issue.

[edit-1]Derp. You were talking about std::basic_string and not std::deque. Yeah, I definitely have to look into this.

[edit-2]I'm changing console.cpp:510 to this:

Code: [Select]

dc_command_buf.erase(dc_command_buf.size() - 1);
Alternatively, I could have gone with resize() with the same argument, but I feel that erase() is clearer in its purpose.

[z64555]

Heh. the patch you made is an SVN patch. Kinda makes things interesting when trying to apply it to a local git repo.

[Echelon9]

Hi,

There's still at least two locations in sexp.cpp and one in starfield.cpp that were identified in the Github comments which are yet to be resolved. See error report from build on your latest push to debug_console branch:

Code: [Select]

code/parse/sexp.cpp:24672:51: error: cannot pass object of non-POD type 'SCP_string' (aka 'basic_string<char,
      std::char_traits<char>, SCP_vm_allocator<char> >') through variadic function; call will abort at runtime [-Wnon-pod-varargs]
        sprintf(sexp_always, "( when ( true ) ( %s ) )", sexp);
                                                         ^
code/parse/sexp.cpp:24675:46: error: cannot pass object of non-POD type 'SCP_string' (aka 'basic_string<char,
      std::char_traits<char>, SCP_vm_allocator<char> >') through variadic function; call will abort at runtime [-Wnon-pod-varargs]
        dc_printf("SEXP '%s' run, sexp_val = %d\n", sexp_always, sexp_val);

code/starfield/starfield.cpp:983:53: error: cannot pass object of non-POD type 'SCP_string' (aka
      'basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >') through variadic function; call will abort at runtime [-Wnon-pod-varargs]
                        dc_printf("Error: unknown flag argument '%s'\n", arg);
                                                                         ^
Also, there appears to be no changes to the windows_stubs/stubs.cpp file as suggested. Recopying the comment you made in relation to this change here again just in case:

There's a DCF_BOOL macro invoked at line 405, this is likely the cause. Since code/windows_stubs/ is not included in the MSVS project filters, I did not catch that. Thanks.

[Echelon9]

Worth keeping in mind re the debug console is that there are in z64555's code plenty of ways to crash the game using the debug console.

For instance, a number of the functions which report values or percentages blindly. That is they do not check first if the game is in a state which would make such a value meaningful, e.g. in mission vs. in menus.

Thus there are a number of ways to crash the engine when using the debug console from the menus, e.g.:

Code: [Select]

set_hull ?

==10060==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000001e8 (pc 0x000102212d8a sp 0x7fff5fbfc740 bp 0x7fff5fbfc830 T0)
    #0 0x102212d89 in dcf_set_hull ship.cpp:13666
    #1 0x10074003e in dc_do_command console.cpp:145
    #2 0x100747296 in debug_console) console.cpp:576
    #3 0x10016e885 in game_poll freespace.cpp:5139
    #4 0x100147d52 in game_check_key freespace.cpp:4934
    #5 0x102515404 in UI_WINDOW::process window.cpp:416
    #6 0x101067bd1 in player_select_do playermenu.cpp:343
    #7 0x100177f1b in game_do_state freespace.cpp:6763
    #8 0x1007b4bbd in gameseq_process_events gamesequence.cpp:409
    #9 0x10017e334 in game_main freespace.cpp:7142
    #10 0x10017f9f8 in SDL_main freespace.cpp:7276


Code: [Select]

set_shield ?

==10010==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000028 (pc 0x000102af59ec sp 0x7fff5fbfc600 bp 0x7fff5fbfc750 T0)
    #0 0x102af59eb in shield_get_strength objectshield.cpp:24
    #1 0x102212815 in dcf_set_shield ship.cpp:13640
    #2 0x10074003e in dc_do_command console.cpp:145
    #3 0x100747296 in debug_console) console.cpp:576
    #4 0x10016e885 in game_poll freespace.cpp:5139
    #5 0x100147d52 in game_check_key freespace.cpp:4934
    #6 0x102515404 in UI_WINDOW::process window.cpp:416
    #7 0x101067bd1 in player_select_do playermenu.cpp:343
    #8 0x100177f1b in game_do_state freespace.cpp:6763
    #9 0x1007b4bbd in gameseq_process_events gamesequence.cpp:409
    #10 0x10017e334 in game_main freespace.cpp:7142
    #11 0x10017f9f8 in SDL_main freespace.cpp:7276

[z64555]

Ah, yes, that does bring up a critical point for the debug console. Just because I've updated the UI and overhauled the parsing routines does not mean the console is 100% safe. When writing new DCF's, the programmer must take in consideration of the values the user will be modifying, and whether or not the subsequent action will through the game into an unknown state.

There's no way for the console to check whether or not a DCF will do such a behavior, it is therefore the responsibility of the DCF's programmer to make sure this doesn't happen, or in the very least make sure to document it in the short or long help strings of the DCF so the user doesn't get caught off gaurd.

Now, with that out of the way, can AddressSanitizer or Coverity help catch some of these problems of the existing DCF's? I think for Coverty to work the debug_console/branch would have to be merged into trunk first.

[Echelon9]

Yes, it's fundamentally up to the DCF programmer to check current state.

AddressSantizer can pick them up, and is the manner in which I've found most so far. It is a runtime based check though, so needs to achieve high code coverage by just trying each DCF in an AddressSanitizer-enabled build.

Once the remaining outstanding code comments have been addressed, I'd be fine for this to go into trunk (given lengthy review).

[z64555]

Alright, commit pushed to branch. If nothing else crops up within 7 days, I'll squash the debug_console branch and commit the changed files to SVN trunk.

[Echelon9]

I went to take a look at your latest commit, and it appears no files were changed?

See here: https://github.com/z64555/fs2open.github.com/commit/6253bd373f34cd086ba7fce140b74bd48519db66

We're you on your local debug_console branch when you pushed similar to: "git push origin debug_console"?

[z64555]

git was acting funny and wanted me to pull from github before I could push the commit to it. here's what the reflog looks like:

Code: [Select]

*   SHA-1: 6253bd373f34cd086ba7fce140b74bd48519db66
|\
| * SHA-1: c9fd0e4a2bfdd9436c90cc4cc4abe8b35aabbcd3
| |
* | SHA-1: ce281814043471e5263c35d86178709a4715fb46
|/
*   SHA-1: 21136e87a5c5601e33c01162c43d0cae648de10b

the two middle commits, c9fd and ce2818 are identical. 

[Edit] Ah, wait a minute! Looks like ce2818 has the stubs on it. I think I might've pushed it to github, amended the commit on my local machine, and then didn't force push the commit to github. Hence the confusion..

Now to figure out how to fix that. :/

[Edit2] Ok, did a hard reset to ce2818 and force-push to /z64555/fs2open.github.com/. Fixed.

[Echelon9]

Thanks for cleaning up the Github commits.

I've given the branch https://github.com/z64555/fs2open.github.com/tree/debug_console another test on OS X. It worked fine, with the additional step of using the patch for Xcode project files which I'd included earlier in this thread. Feel free to add that patch to your branch, then it's fine to commit to SVN trunk (note we are yet to formally move our main code hosting to Github).

Do you need someone with commit privileges to push the debug_console changes to SVN for you?

[z64555]

Figured out how to apply your SVN patch to my GIT repo. Not sure why Git's Bash doesn't have help for 'patch'

Echelon9, I can commit to SVN tomorrowish. The move to github won't be until antipodes is merged, I think.

[chief1983]

I thought the git command to apply a patch is 'apply', not 'patch'.