Jump to content
The Dark Mod Forums

Dubious use of postfix increment operator (possible bug)


Hamlet
 Share

Recommended Posts

While analysing the warnings emitted by GCC 6, I was pointed to this piece of code in renderer/Model_lwo.cpp (comment added):

int sgetI1( unsigned char **bp )
{
   int i;

   if ( flen == FLEN_ERROR ) return 0;
   i = **bp;
   if ( i > 127 ) i -= 256;
   flen += 1;
   *bp++;       // <== warning: unused value
   return i;
}


short sgetI2( unsigned char **bp )
{
   short i;

   if ( flen == FLEN_ERROR ) return 0;
   memcpy( &i, *bp, 2 );
   BigRevBytes( &i, 2, 1 );
   flen += 2;
   *bp += 2;
   return i;
} 
Starting from the second function, sgetI2(), it appears that it receives as argument a pointer to an unsigned character, passed by reference in C style (that becomes a pointer to the pointer).

It copies two bytes in a short, swap them as needed (handling little/big endian, I suppose), then it makes the pointer *bp point after the copied data and returns the value read.

There is a sgetI4() which does the same with 4 byte data.

Back to the first quoted, I was assuming sgetI1() would do the same. GCC complains that the statement *bpp++ produces an unused value. It turns out, the postfix increment operator (a++) has the highest priority among C and C++ operators, and it gets executed before the dereference operator. That means that I would expect the code to operate like (*bp)++, equivalent to *bp += 1, similarly to sgetI2(), but I get instead a *(bp++). This means that the local variable bp (unsigned char**) is increased (while the unsigned char* pointer *bp is not), and then the value it was pointing to before the increase is taken and ignored.

All in all, it means that when it's called as sgetI1(&ptrToBuffer), it returns the value at the current value of ptrToBuffer and, on the next call, sgetI1(&ptrToBuffer), it returns again the same value, since ptrToBuffer is not increased.

 

The same construct appears in sgetU1() and in add_clip() on nclips (a simple pointer int*), where it might have more serious consequences:

   *nclips++;
   clip->index = *nclips;
since the an index is assigned from the cell of memory next to nclips (nclips is first increased).

I can't find any place where the former two are used, while add_clip() is pulled in from LoadLWO() in renderer/Model.cpp (at which point I stopped tracking).

 

I attach a trivial patch that removes these warnings by just doing the thing that would be usually expected (increase the pointed value) (it also removes the other instance of it, not mentioned in this text).

 

 

Edit: discovered that syntax highlight for C/C++ works with code type "auto", even if it does not show in the preview.

UnusedValue.patch.txt

Edited by Hamlet
  • Like 2
Link to comment
Share on other sites

Is this code ever executed and does the fixed code cause any instantly visible problems?

Not sure if TDM uses LWO's anywhere?

My understanding is too small to answer the first question: I learned what LWO means after reading the previous post.

I also don't know what a "clip" is.

The main reason of my post is to see if the experts have anything to add to the picture, that might predict a type of failure or to connect this to an already observed one.

I can see the dubious function being called when a ID_RIMG or ID_TIMG is found, in code that looks like a parser. The former seems related to reflection effects on a surface, the latter ends in the parameters of a texture. Maybe there might be effects related to clipping of these surfaces and textures? Just wildly guessing here.

 

About the patch: I do not claim to have tested it properly yet, and as always I personally recommend not merging it into anything official until that happens.

Link to comment
Share on other sites

Clip is a special texture that imbues a surface with physics attributes.

Plain clip is used for the collision model, playerclip keeps a the player from

entering an area, and monsterclip is used to ensure that AI can properly pathfind

by simplifying the path generation.

Please visit TDM's IndieDB site and help promote the mod:

 

http://www.indiedb.com/mods/the-dark-mod

 

(Yeah, shameless promotion... but traffic is traffic folks...)

Link to comment
Share on other sites

Hamlet,

I'm a little late with this advice, but I've just loaded a TDM map in DarkRadiant (DR) for the 1st time. And since you've already installed DR, if you extract one of mission PK4 files (I used the simplistic "Closemouthed Shadows" mission) and load the map in DR, you'll get a nice window showing various "Clip" and "Monster Clip" (etc) objects, rendered in 3D and nicely labeled.

As for your patches to address these compile-time warnings, I have not forgotten about them. I plan to look into any/all of them more sometime after the 2.05 build problems are put to rest. I think some of these changes may meet some resistance, but I still think they're worth further investigation, so I hope you haven't begun to think that I'm ignoring your patches. There's just so much to do and so little time (for me).

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

 Share

  • Recent Status Updates

    • freyk

      Some years earlier i created some launchers for TDM. uploaded today some new sourcecode and executables on my github repo.
       
      · 0 replies
    • Epifire

      Say, I know it's been a while since the site got overhauled from the crash. But did we ever figure out if/how to get the recent topics & replies list back? It's not a total deal breaker but it was nice for becoming a thread creeper again...


      · 2 replies
    • Epifire

      Some of you who've been on the TDM discord know I was out of work last Winter, just putting in hrs for the developer portfolio. Currently I've been working a seasonal job to pay the bills and now I'm finally in my last week before I get bumped off. Things will be tight but I'm planning a long off period to make as much content as I can. Big plans in Unreal Engine as well as my most ambitious TDM collab yet! Never been so excited to be a stuck at home to pursue my life's work. With a lotta time and maybe some luck, I'm hoping to get enough art work done that I may start applying around to studios.
      · 4 replies
    • STiFU

      I finally got around to play Prey and I truly loved it. It is an incredible homage to System Shock and Deus Ex. While the gameplay is not en par with those two titles, the game makes up for that with its well written lore and story. The whole "world" just feells so authentic and it features a ton of really god environmental story-telling. Recommended for every immersive sim fan, i.e., everyone on this forum.
      · 2 replies
    • jaxa

      Alder Lake has arrived:
      https://forums.thedarkmod.com/index.php?/topic/18055-2016-cpugpu-news/page/15/&tab=comments#comment-466190
       
      · 0 replies
×
×
  • Create New...