Hard Light Productions Forums

Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Test Builds => Topic started by: z64555 on February 19, 2014, 01:09:35 pm

Title: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on February 19, 2014, 01:09:35 pm

(https://dl.dropboxusercontent.com/u/97612992/FreeSpace2%20Open/SCP/z64_debug_console.png)

(http://scp.indiegames.us/img/windows-icon.png) WINDOWS Test Build
Compiled on MSVC 2012 update 4
Debug Console Test Build (https://dl.dropboxusercontent.com/u/97612992/FreeSpace2%20Open/SCP/Debug%20Console/fs2_open_3_7_1-DEBUG.exe) (based on r10475)

github branch
/fork-z64555/debug_console (https://github.com/z64555/fs2open.github.com/tree/debug_console)

Short Tutorial of Use:
1. Run the build.
2. Get to the login screen.
3. Press Shift+Enter to bring up the console.
4. Type in and enter help To list available commands and general usage.

You may use the same key combination (Shift+Enter) almost anywhere in the game to bring up the console, this will currently pause game execution until you leave the console.

CAUTION: Usage of the debug console in multiplayer games may crash to desktop or otherwise cause unexpected behavior. This will eventually^TM change.

It is planned to separate the console to a different thread, so that the game may run in the background. There will be a "pause" command to pause/unpause the game.

(rambling drivel follows)

---

-=Introduction=-

When I first discovered the FreeSpace Open Debug Console in November of 2013, I found it to be the answer to the problem of needing a way with directly interacting with the FSO engine, especially in terms of debugging new features. The debug console allows developers to check how things are going here and there, all that was needed was run a debug build, get past the opening credits, and hit Shift+Enter almost anywhere in the game to access the console. From here, they could monitor the value of a specific variable and even set them to any arbitrary value.

The F1 help overlay, for instance, has a number of DCFs (debug console functions) to adjust the polylines that draw between the help text and the region of button that they're describing. With a bit of work, something like this could be done for the HUD gauges to aide in their design, in-game. A developer could try nudging the ETS gauge here and there and take it for a test run, and not have to end the game, edit a .tbl, and reboot to see the changes. The debug console also allows testing of otherwise difficult or complex aspects of the codebase. It appears a number of multiplayer-related DCFs were implemented by them to test their lag simulation and how well the engine handled the vigors of networking. Without the debug console, these functions are otherwise inaccessible and useless.

However, by the time I found the console, it was ill maintained and little known. The interface was an abysmal 80x25 letterbox on any resolution of the game, and could only scroll down through a page at a time through the help list. Add to that, the console's command line parser had little to no type safety, making it quite easy to CTD over an otherwise small mistake of entering a float when a DCF needed an integer, or a long when it needed a ubyte. If the console was to live up to its potential, something drastic needed to be done.

Like, a complete rewrite of the parser, and a brand new renderer for the console itself.

-=Overview of Changes=-

The parser now uses a syntax and grammar quite similar to what's found in parselo, and features heavy usage of dc_optional_string's and dc_maybe_stuff's. The latter mentioned are, what I think, novel in grabbing optional arguments from the command line.

The console renderer is still in-game at the moment. A future goal for the console is obviously to have its own window, as with the debug_window that spews the fs2_open.log, but that's outside the scope of this project. (a.k.a. I don't have enough time to fool with it just yet)

The console renderer now features some essentials, like a proper word wrap and the ability for tab alignment (although with FSO's non-monospace fonts, that's pretty much pointless at this time), and the ability to scroll through the output history. Also, the console takes up the entirety of the FSO's screen, and is no longer limited to the 80x25 letterbox.

I tried as much as I could to update the existing DCFs to use the new parsing functions, but there may be bits here and there that I might have missed. I've included quite a few doxygen comments, too. Most should be up to date, but again I might've missed some bits here and there. This will change within the week or so.

-=Ending Note=-

Closing out, I'd like to have as many people looking at this as possible, because I feel that the console is just too good of a tool to _not_ use. Thanks for your interest. :)

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Goober5000 on February 19, 2014, 03:20:07 pm

Ooh, this sounds encouraging. :)

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on February 19, 2014, 06:41:54 pm

I've been having some trouble generating a proper .diff. The situation is a bit weird, as I started working on the debug console while in the antipodes branch. I had to do some finagling of things in order to get the .diff to cooperate with the SVN trunk, but it appears issues are still arising here and there.

Please let me know if any rejected hunks crop up, and I'll do my best to resolve them.

[edit] First revision. Looks like the .diff I originally linked missed a couple of added files. I tried going onto dropbox to update, but for some reason I can't access them at the moment. So, until that gets resolved I'll be using pastebin. Just download the paste as a .diff and delete the /* EOF line, please ignore*/ and the very bottom, but be careful to not delete the EOF line that's right above it.

Link: http://pastebin.com/JahgCFSy (http://pastebin.com/JahgCFSy)

Once dropbox gets back online, I'll also include a test build... at which point this thread should probably be moved into the test build subforum (hint hint)  :P

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on February 23, 2014, 06:02:36 pm

:bump:

Updating OP with a test build.

Would a moderator plz move this to the test builds subforum? pretty plz?  ;)

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: ngld on March 10, 2014, 12:11:33 am

I've tried to compile this on Linux but the gcc aborts with this error:

Code: [Select]

debugconsole/console.cpp: In function 'void dc_do_command(SCP_string*)':
debugconsole/console.cpp:266:49: error: cannot pass objects of non-trivially-copyable type 'SCP_string {aka class std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >}' through '...'
   dc_printf("Command not found: '%s'\n", command);
                                                 ^
debugconsole/console.cpp:274:70: error: cannot pass objects of non-trivially-copyable type 'SCP_string {aka class std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >}' through '...'
   dc_printf( "Ignoring the unused command line tail '%s'\n", command );

BTW, the link to the GIT diff is wrong... get_flags_wiki ?

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Goober5000 on March 10, 2014, 01:33:29 am

Nicely spotted, ngld.  This is why we need people to do code reviews. :D

The printf and sprintf functions (and indeed, any function with varargs) cannot be used with classes, including SCP_string.  The line will need to be changed to pass command.c_str() instead of command.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: ngld on March 10, 2014, 03:14:10 am

I've fixed most problems (updated diff (http://pastebin.com/dS87APbE)) and it compiles by now.
However, once you try to run fs2_open, it instantly crashes. The following error message is what I got from valgrind:

Code: [Select]

==7742== Invalid read of size 8
==7742==    at 0x70ECB3A: ??? (in /usr/lib/libstdc++.so.6.0.19)
==7742==    by 0x4A78FE: std::_Rb_tree_iterator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >::operator--() (stl_tree.h:204)
==7742==    by 0x4A783A: std::_Rb_tree<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*>, std::_Select1st<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::_M_get_insert_unique_pos(std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const&) (stl_tree.h:1333)
==7742==    by 0x4A7222: std::_Rb_tree<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*>, std::_Select1st<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::_M_get_insert_hint_unique_pos(std::_Rb_tree_const_iterator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const&) (stl_tree.h:1425)
==7742==    by 0x4A6F9E: std::_Rb_tree<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*>, std::_Select1st<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::_M_insert_unique_(std::_Rb_tree_const_iterator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> const&) (stl_tree.h:1478)
==7742==    by 0x4A6C3D: std::map<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, debug_command*, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::insert(std::_Rb_tree_iterator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> >, std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> const&) (stl_map.h:648)
==7742==    by 0x4A68BA: std::map<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> >, debug_command*, std::less<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > >, SCP_vm_allocator<std::pair<std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const, debug_command*> > >::operator[](std::basic_string<char, std::char_traits<char>, SCP_vm_allocator<char> > const&) (stl_map.h:469)
==7742==    by 0x4A6102: debug_command::debug_command(char*, char*, void (*)()) (consolecmds.cpp:37)
==7742==    by 0x41B10D: __static_initialization_and_destruction_0(int, int) (freespace.cpp:669)
==7742==    by 0x41B5EB: _GLOBAL__sub_I_Env_cubemap_drawn (freespace.cpp:8942)
==7742==    by 0x916FEC: __libc_csu_init (in /home/tobias/games/fs2/fs2_open_3.7.1_DEBUG)
==7742==    by 0x75B3A94: (below main) (in /usr/lib/libc-2.19.so)
==7742==  Address 0x8 is not stack'd, malloc'd or (recently) free'd

It seems this is caused by "dc_commands[_name] = this;" on line 37 of consolecmds.cpp.
I have no idea what exactly is going wrong here...

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: m!m on March 10, 2014, 04:19:37 am

@ngld: Your patch does not contain any changes which are not in the code folder, can you recreate the patch so it includes the changes to the Visual Studio project files?

@z64555: I looked at the patch and I noticed some things that you might want to fix:

• You are using plain char* pointers in a loot of places where you should use const char*, always use const char* if you don't actually need to change the string as using char* pointers with string literals and then changing them causes undefined behavior.
• You have a lot of duplicated code in consoleparse.cpp which only differs in that one uses a c-style string and one uses a SCP_string, removing the c-style version and patching all usages of it will make it easier to add internationalization later.
• This is more a personal opinion and nothing set in stone but using a pointer to a variable to pass return values and then not actually checking for a NULL-pointer is dangerous and should in most cases be replaced by a reference as that will also catch many other cases of wrong usage.
• Is there a reason why you are using 8 spaces to indent? I had the impression that 4 spaces or a tab were the "default" style in the FSO code although we don't actually have an official coding style

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on March 10, 2014, 07:34:09 am

@ngld: I updated the GIT patch not too long ago, that might've been what confused Pastebin and sent you to a different paste (that's actually a rough draft for a fso cmdline option).

The link should now point to the intended patch.

@m|m:

• Good catch on the char * vs. const char *. I'll see if I get that changed today.
• I was going by the example put forth by parselo.cpp, I wasn't sure what was the prefered route so I went with both until that was settled.
• Hmm, was pretty sure I got all of the nullptr checks in place, but it looks like a few functions slipped through without getting them.
• Uh, where is it 8 space indented?

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: m!m on March 10, 2014, 07:48:35 am

In my patched checkout consoleparse.cpp is indented with 8 spaces but it might be an issue with the patch files.

Also I just noticed you are using a class called errParse to report parsing errors but you are not deriving it from std::exception which should be done in most cases.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Goober5000 on March 10, 2014, 11:20:35 am

The example in parselo.cpp, where there are both SCP_string and char* versions of string processing functions, is because we want to be able to use the functions on both sources.  I'm currently in the middle of a project to convert the mission loading code to use SCP_string, and I use both versions of the parselo functions in various places.

Whether to maintain parallel versions of the functions in this patch depends on the intended use.  Based on m!m's comment "...removing all uses of [the c-style version] and patching all usages of it", it sounds like there are quite a lot of references to both throughout the code, so you should leave both versions in for now.  Focus on getting the functionality working first, and then at some future date, you can refactor the invocations if you need to.

Regarding "using a pointer to a variable to pass return values ", I haven't checked the diff, but is this used in the function parameter list or in the return statement?  If it's in the latter, you would be returning a pointer to a variable on the stack and that is a big no-no.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: m!m on March 10, 2014, 11:25:57 am

Quote from: Goober5000 on March 10, 2014, 11:20:35 am

[...]  Based on m!m's comment "...removing all uses of [the c-style version] and patching all usages of it", it sounds like there are quite a lot of references to both throughout the code, so you should leave both versions in for now.  [...]

I haven't actually checked which version is used, I only noticed that there was quite a lot of duplicated code because of this and I though now would be a better time to unify it to use the SCP_string version instead of waiting and then never doing it.

Quote from: Goober5000 on March 10, 2014, 11:20:35 am

Regarding "using a pointer to a variable to pass return values ", I haven't checked the diff, but is this used in the function parameter list or in the return statement?  If it's in the latter, you would be returning a pointer to a variable on the stack and that is a big no-no.

I was talking about pointers in the argument list.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Goober5000 on March 10, 2014, 11:40:04 am

Quote from: m!m on March 10, 2014, 11:25:57 am

I haven't actually checked which version is used, I only noticed that there was quite a lot of duplicated code because of this and I though now would be a better time to unify it to use the SCP_string version instead of waiting and then never doing it.

There's no harm in leaving it alone for now.  In contrast, postponing it has the advantage of separating conceptual blocks of work into different patches, and doing it now has the disadvantage of making the diff more complex and risking the introduction of bugs.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: ngld on March 10, 2014, 01:14:17 pm

I've updated the GIT patch. (http://pastebin.com/WYqLQ32b)

• Added c_str() every time a SCP_string is passed to dc_printf().
• Added #include <limits.h> to consoleparse.cpp for INT_MAX.
• Added #include "debugconsole/console.h" to windows_stubs/stubs.cpp because it uses DCF_BOOL.
• Changed dc_command_buf.pop_back() to dc_command_buf.erase(dc_command_buf.end() - 1); since pop_back() is C++11
• Added all new files in code/debugconsole to code/Makefile.am

FSO now compiles and works correctly but the console has strange issues when printing text...
(http://i.imgur.com/eF3grR9.png)
The prompt always displays correctly but once I press enter the last output line (which wasn't visible before) appears on the same line as the prompt...

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on March 11, 2014, 08:23:23 am

@ngld: It appears that some functions that use dc_printf don't have the needed '\n' at the end, leaving the printf cursor at the end of the last line printed instead of at a new line. When a new command is entered from the command prompt, the string is pushed onto the output buffer, and thus, collides with the last line.

Hmm, see_all is likely a DCF_BOOL function, so that may be one of my mistakes there.  :rolleyes:

[amend1] I'll see if I can get a github repo set up. This exchange of .patch's is just too cumbersome.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on March 14, 2014, 11:55:16 pm

:bump:

Alright, trying to pass along a Git .diff and an SVN .diff proved to be too much of a hassle. I've finally got my git-svn local repo set up so that it can push to either the SVN, the github remote, or my personal fork. Once code review completes I'll merge the debug_console branch into the master branch, and then either Dcommit it into the SVN or pull it into the GIT origin, depending on which remote will be considered the official at the time.

Here's my fork: fork-z64555/debug_console (https://github.com/z64555/fs2open.github.com/tree/debug_console)

I just finished doing the merge from the antipodes branch where I originally started work on the debug console. Let it be known that I am never goign to try making a .diff by hand ever again, because it's just too big a pain in the ass to do, and a bigger pain in the ass to try to correct.

Notable changes:

• String parsing functions that stuffed to a char[] have been dropped in favor of the duplicate functions that use SCP_string instead.
• Stuffing functions have been largely changed from accepting a * to a & reference.
• SCP_strings that are used in dc_printf's have been postscripted by the proper .c_str()

[amend] I updated the OP to reflect the switch to the github branch. I'll see if I can upload a debug build of the the newest commit soon.

[amend_1]OP updated again, this time I updated the link to point to the newest build.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Echelon9 on April 18, 2014, 02:32:00 am

Hi - Are you able to rebase this against the latest scp git branch? Reading through the comments this is as against the antipodes branch am I correct in thinking?

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on April 18, 2014, 01:51:58 pm

It was originally based against antipodes, but after some learning experiences I managed to get it based on remotes/origin. That's what's on the fork right now.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: m!m on April 18, 2014, 01:55:47 pm

The problem is that your debug console branch is not based on any branch from the main repository. The easiest way to fix this is probably to just create a new based in the antipodes branch from the main repo and apply your patch to that branch.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on April 18, 2014, 03:11:18 pm

Alright. Don't try to pull anything from my fork. I'm in the middle of some management.

:bump: Sanitized the git repo, and set everything up as it should. I've seperated the svn and git repos, so this shouldn't be a problem anymore.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Echelon9 on April 20, 2014, 04:26:30 am

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.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on April 22, 2014, 08:01:32 am

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.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Yarn on April 22, 2014, 11:57:37 pm

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".)

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on April 23, 2014, 06:59:22 am

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.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: m!m on April 23, 2014, 07:51:04 am

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

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Echelon9 on April 23, 2014, 10:04:11 am

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

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on April 23, 2014, 10:25:45 am

 :banghead:

Somehow those files got ignored at some point.

New commit Pushed to fork.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Echelon9 on April 23, 2014, 11:48:50 am

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]

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on April 23, 2014, 12:34:23 pm

Quote

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

 :banghead: This issue keeps resurfacing. Thanks for spotting them.

Quote

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. :)

Quote

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.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on April 23, 2014, 02:40:25 pm

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

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Echelon9 on April 24, 2014, 08:24:28 am

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:

Quote

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.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Echelon9 on April 24, 2014, 08:59:04 am

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

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on April 25, 2014, 07:05:56 am

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.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Echelon9 on April 25, 2014, 08:02:13 am

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).

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on April 28, 2014, 06:28:23 am

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.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Echelon9 on April 28, 2014, 10:00:28 am

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"?

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on May 02, 2014, 02:53:06 pm

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.  :confused:

[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.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: Echelon9 on May 04, 2014, 01:11:27 am

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?

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on May 04, 2014, 02:53:14 pm

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.

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: chief1983 on May 05, 2014, 10:57:40 am

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

Title: Re: [CODE REVIEW] z64's Debug Console changes
Post by: z64555 on May 05, 2014, 11:03:06 pm

And committed. Thanks to chief1983, The_E and others for pointing out it's a good idea to change the project files so that added files are actually compiled. Oh, and to keep an eye on git and svn regarding new files, as they sometimes get ignored and can't be officially added.  :banghead:

Letsee, aside from the future plans for making the debug console concurrent with FSO execution, there's some lesser plans to clean up some 'trivial' warnings here and there.