18 replies to this topic

[Hamlet]

Hello all,

 

this is not exactly a generic offer for help...

 

While compiling The Dark Mod with GCC on Linux I have witnessed an endless stream of warnings.

I would like to resolve those warnings so that the code becomes standard C++.

And I would like to know if the project is interested in the resulting patches.

I have started already. Just because, believe it or not, for me that's somehow fun.

I have enabled all warnings and pedantry, identified about 30 types of warning, and I am bashing them one by one.

 

An example of motivation: GCC (6.2) tells me:

renderer/Model_ase.cpp: In function ‘void ASE_KeyGEOMOBJECT(const char*)’:
renderer/Model_ase.cpp:724:37: error: argument to ‘sizeof’ in ‘void* memset(void*, int, size_t)’ call is the same expression as the destination; did you mean to dereference it? [-Werror=sizeof-pointer-memaccess]
   memset( ase.currentMesh, 0, sizeof( ase.currentMesh ) );
                                     ^

That is: it finds a bug (or at least it looks so to me), and it even tells the right solution. And this warning hasn't been noticed because swamped in others that are surely harmless.

Of course this example shows also the limit of the approach: what if that unusual statement was intentional? I would be introducing a bug instead.

So I may need to ask some questions along the way.

 

I intend to direct my effort to ISO C++ 2003 compliance first, and then consider ISO C++ 2011, if the project thinks it's ok. But that will be far from now.

I also do not intend to touch the C code, because I am not as fluent there. This also means that the result should be welcome by any compliant compiler on any platform, and by now I suppose even MicroSoft compilers are compliant to C++03.

 

I can imagine the project would rather see this energy devoted to other items in the to-do list... but in my mind this one is a necessary step toward bug fixing.

Ball in your field, comments are welcome.

[duzenko]

I don't like warnings-laden code as well.

However in that particular example with sizeof, what would be the fix?

That looks like a bug - but who knows what the fix is - clear the pointer, dealloc mem or zero fill ? Or maybe comment out that line altogether?

[grayman]

Since fixing Linux warnings affects the windows build, and msvs is currently flagging very few warnings (some of which don't need fixing), I 'm against a wholesale removal of warnings.

I'd rather see time and energy spent fixing bugs. If a warning is a bug, please point to the bug as the player experiences it before changing anything. bottom line: id didn't write perfect code, but it works, and we don't have the coders to chase down new bugs introduced by a warnings purge.

[NightStalker]

Excellent catch, Hamlet!  I went back and looked through some old compiler output logs and that warning has been there all along and I never saw it because, in further support of your point, one simply cannot easily see the critically important compiler warnings because they're buried in so much noise!

I'm a bit embarrassed that I missed this serious warning.  I was intending to start looking into all those warnings at some point, once the 2.05-beta issues (and some other issues) are fully addressed.  Given the presence of such a bug that the compiler was screaming at me about, only for me to ignore it, I'm thinking that I should accelerate that task a bit, at least to the extent that I'm not "stepping on your toes" or otherwise interfering.

Grayman, I sympathize with your position and viewpoint, but only up to a point.  I think this is an excellent example of why we need to at least begin to address all (and hopefully, eventually, eliminate as many) of these warnings as we possibly can.

It sounds like in this case, the GCC compiler is more capable than the Windows compiler, even though this bug (and make no mistake, this is a bug!) affects all OSes.  I have a feeling that, overall, the GCC compiler may be much more thorough than the Windows compiler, but I have no hard evidence for this, only my experience over many years of the GCC suite getting better and better at finding problems in my (and others') code.  If the Windows compiler has the equivalent of the GCC compiler options "-Wall" ("enables all the warnings about constructions that some users consider questionable") and/or "-Wextra" ("enables some extra warning flags that are not enabled by -Wall"), then it might be wise to enable them (assuming they aren't already), to see what folks compiling under Windows may have been missing for a while.  In fact, I'd ask anyone compiling TDM under Windows (duzenko, maybe?) to check: Did it warn about this particular "memset()" call?

I wanted to have a bit more hard evidence for my position about fixing warnings, so I added a couple of lines to 'Model_ase.cpp' on the 2.05-beta build, compiled it, ran TDM, and loaded up the "Flakebridge Monastery" mission (simply because that's what I last used in my "Baal crash" testing).  I found that the structure being zeroed is a 96-byte structure, but with the bug, it's only clearing 4 of the 96 bytes.  And, more importantly, just loading (not even playing) the mission, that routine is called 547 times, with that particular erroneous 'memset()' call happening a whopping 80 times!  Now maybe it turns out to be harmless -- this part of the code is about "meshes" and "animation" and I'm not smart enough on modeling to understand the real effect of this bug, but it clearly needs to be fixed.  On that, I am unbending.

BTW, this bug is in the original Doom3 engine's code.

I'd encourage a "middle ground" here.  Let's fix these warnings, but slowly and carefully (not that Hamlet was suggesting otherwise).

Hamlet, as you know, I'm very grateful to have another Linux builder commenting and contributing, especially one with your skill level, especially in C++, however frequently you might be inclined to post here.  I'm of the same mindset as you are and I'd like to improve TDM as much as reasonably possible, over the course of the next several weeks or months, as time is available.

Grayman, I hope you'll agree that there is inherent merit to addressing these warnings, without always having to "point to the bug as the player experiences it before changing anything".  In all of my myriad projects, all compiled with GCC, I generally don't ever allow any warnings in a build, frequently even enforcing that with the "-Werror" ("Make all warnings into errors.") GCC compiler option.

Warnings are often pointing out a very nasty, sometimes insidious bug.  I've seen far too many projects casually ignore compiler warnings.  Please, let's not be one of them, at least not in the long run.

As for fixing bugs before warnings, I don't disagree a bit, but I've scanned the BugTracker and don't see anything that I am capable of addressing.  So my plan is to publish the info (later today, I hope) about Baal's crash and then start looking into an oddity (possible bug) in the "latest SVN" build for which I have not had time to confirm if it's also in 2.05-beta.  If there are other open bugs within my grasp that I'm unaware of, definitely, please, let me know.

Finally, can anyone who understands animations and meshes comment: what might the effect be of not fully clearing (zeroing) out a "mesh" in code with the brief comment "ignore regular meshes that aren't part of animation"?

 

[nbohr1more]

Based on conventions elsewhere in the code, it looks like this what is done:
 
 

 
memset( ase.currentMesh, 0, sizeof(aseMesh_t));
 

[NightStalker]

Based on conventions elsewhere in the code, it looks like this what is done:

memset( ase.currentMesh, 0, sizeof(aseMesh_t));

Yes, that's what seems to be intended.

 

I should have mentioned in my previous post that this is the line I entered (in 2 spots, with a unique counter to distinguish code position):

printf("NightStalker:{1|2} sizeof( ase.currentMesh ) = %d   sizeof( aseMesh_t ) = %d\n", sizeof( ase.currentMesh ), sizeof( aseMesh_t ));

That structure is 96 bytes.

 

But whoever fixes this bug should still take a careful look to make sure that the intent of the original coder was truly as we suspect (i.e. don't anyone "rush in" an SVN commit without carefully checking it, please!).

Edited by NightStalker, 29 December 2016 - 02:22 PM.

[nbohr1more]

Hmm. When I tried this, tables in NHAT mission 1 became black.

 

I then just commented-out the line:

 
// memset( ase.currentMesh, 0, sizeof( ase.currentMesh ) );
 

and things went back to normal. Gonna check a few Doom 3 branches for any changes.

 

Edit: Most vanilla branches have this strange memset and it even exists in the BFG codebase BUT

the Dhewm branch has removed the memset. That's a pretty good endorsement for it's removal.

[duzenko]

It sounds like in this case, the GCC compiler is more capable than the Windows compiler, even though this bug (and make no mistake, this is a bug!) affects all OSes.  I have a feeling that, overall, the GCC compiler may be much more thorough than the Windows compiler, but I have no hard evidence for this, only my experience over many years of the GCC suite getting better and better at finding problems in my (and others') code.  If the Windows compiler has the equivalent of the GCC compiler options "-Wall" ("enables all the warnings about constructions that some users consider questionable") and/or "-Wextra" ("enables some extra warning flags that are not enabled by -Wall"), then it might be wise to enable them (assuming they aren't already), to see what folks compiling under Windows may have been missing for a while.  In fact, I'd ask anyone compiling TDM under Windows (duzenko, maybe?) to check: Did it warn about this particular "memset()" call?

With EnableAllWarnings (/Wall) MSVC swallows Model_ase.cpp quietly.

 

the Dhewm branch has removed the memset. That's a pretty good endorsement for it's removal.

Agree.

[NightStalker]

With EnableAllWarnings (/Wall) MSVC swallows Model_ase.cpp quietly.

Many thanks for running that test, duzenko!  This is perfect evidence that even the Windows build can benefit from the TDM source code having been compiled for Linux with GCC and its stricter (at least in this case) checks.

I won't argue if you guys are comfortable removing that 'memset()' line, but I still think that there was a reason for it in the original code, even if we just don't know what it was.  (I'd never even heard of the 'dhewm' project until about 3 weeks ago and I have no idea how robust its developers are.)

nbohr1more, I will probably run the same experiment you did when I get a free minute, but can you please clarify... What do you mean by black "tables"?  Do you literally mean the things in the game that one would find chairs around?

If so, then I wonder if removing that line of code might not have just created a problem somewhere else.  And my suspicion is that tweaking that line should have affected a lot more than just tables, given how prevalent the calls to it were.  I really wish I knew more about modeling and that whole side of TDM.  Maybe one day I'll get Dark Radiant to compile under Linux and can learn more.

Edited by NightStalker, 30 December 2016 - 03:42 PM.

[nbohr1more]

Using my line caused Dining Table models to become black.

Removing the existing line, as was done in Dhewm, caused no problems. So it seems that Dhewm's solution is superior.

[NightStalker]

Using my line caused Dining Table models to become black.

OK, thanks!  Now I know what to look for.

 

But as for the line of code, aren't you at least a little curious what it was supposed to do?  And therefore just a little afraid to remove it?

[nbohr1more]

As I can tell, it looks like an attempt to conserve memory resources when instantiating models in a view. Since it appears to be invalid syntax anyway, it's a choice between not functioning with an error and simply not existing in the first place.

[Hamlet]

I have learned a few other things from reading (and "fixing") the warnings. Of the "bug candidate" type. Which would the more proper place/way to discuss them be?

 

P.S. I have not investigated the memset change yet.

[NightStalker]

I have learned a few other things from reading (and "fixing") the warnings. Of the "bug candidate" type. Which would the more proper place/way to discuss them be?

Hamlet, I was hoping one of the "long timers" would've commented by now, because I had the same sort of question.  But since nobody has commented, I will, FWIW.  I'm fine with wherever/however you decide to discuss these issues.  I will find the information that you post regardless.
 
I played around with that 'memcpy()' 'memset()' a bit yesterday, in several missions.
 
@nbohr1more: I finally got to see that dining table in NHAT that you mentioned.  Just the top of the table was black. How did you discover that?  That is, did you know to look for that particular object or did you just happen to see that?  I was trying to determine items in other missions (i.e. in any of the few missions that I've actually played) that use the "*MESH" entry in the ASE file, but there seemed to be no correlation to an item using "*MESH" and the altered in-game rendering.  I know I'm missing something and, even though it's probably not important, I'd like to understand this whole "mesh" thing better.  But, for the record, I see why you want to remove that 'memset()' call and I cannot disagree.  I'm actually wondering if the line above it serves any purpose now, and if so, what?  Just curious, really.  I suppose I should take a look at the dhewm implementation rather than idly wondering, though.
 
EDIT: dhewm3 leaves that other line intact:

// ignore regular meshes that aren't part of animation
else if ( !strcmp( token, "*MESH" ) )
{
	ase.currentMesh = &ase.currentObject->mesh;
	ASE_ParseBracedBlock( ASE_KeyMESH );
}

Edited by NightStalker, 31 December 2016 - 11:09 AM.

[grayman]

We longtimers are old and we move slowly. 

 

Have your discussion wherever it feels right. Here, or in a new thread, is fine.

[Obsttorte]

Since fixing Linux warnings affects the windows build, and msvs is currently flagging very few warnings (some of which don't need fixing), I 'm against a wholesale removal of warnings.

I'd rather see time and energy spent fixing bugs. If a warning is a bug, please point to the bug as the player experiences it before changing anything. bottom line: id didn't write perfect code, but it works, and we don't have the coders to chase down new bugs introduced by a warnings purge.

+1

[NightStalker]

@nbohr1more: I finally got to see that dining table in NHAT that you mentioned.  Just the top of the table was black. How did you discover that?  That is, did you know to look for that particular object or did you just happen to see that?  I was trying to determine items in other missions (i.e. in any of the few missions that I've actually played) that use the "*MESH" entry in the ASE file, but there seemed to be no correlation to an item using "*MESH" and the altered in-game rendering.  I know I'm missing something and, even though it's probably not important, I'd like to understand this whole "mesh" thing better.

Bump...

[nbohr1more]

Honestly, that was pure luck.

 

I know that Biker has made a number of brush-to-ASE models in the missions he worked on.

 

I tested NHAT because it's a BIG diverse mission with lots of assets and because it has an in-game cinematic

so I can rely on it for performance comparisons.

 

If I didn't test NHAT, I would've tested Penny Dreadful 3.

[NightStalker]

Thanks much for the explanation!

 

To assist in my overall understanding of modeling stuff, I've been trying to get DarkRadiant to compile and run under Linux but, as you may see from my new threads in that forum, the "compile" part went well but the "run" part... not so much.