Hard Light Productions Forums

Off-Topic Discussion => Programming => Topic started by: portej05 on October 24, 2009, 11:11:58 am

Title: A subtle bug for you all
Post by: portej05 on October 24, 2009, 11:11:58 am
Lets play spot the bug in this section of code:
Code: [Select]
// HACK - Create a dummy structure to pass to the XyzConnect
// function to avoid AV within the function.
int dummy = 0;   
if ( NO_ERROR != ( XyzConnect( 0, L"", L"", (void**)&dummy ) )
{
    TRACE( L"XyzConnect failed." );
    return FALSE;
}


A hint: Whether or not a crash occurs is dependent on the order that variables are initialised in the function with that code.

If you think you've spotted the answer, click here to find out (http://blogs.msdn.com/oldnewthing/archive/2009/10/23/9911891.aspx).

This is done all over the place in the FSO engine, and is one reason why it needs to be cleaned up if we ever want to get a 64bit build working.
Title: Re: A subtle bug for you all
Post by: Mongoose on October 24, 2009, 12:10:23 pm
So something that works just fine in a 32-bit environment can crash and burn in a 64-bit environment?  That seems foolishly shortsighted from a standards standpoint.
Title: Re: A subtle bug for you all
Post by: portej05 on October 24, 2009, 12:25:22 pm
It is actually spelt out in the standards :P

int doesn't have to be any particular size

From section 5.2.4.2.1 'Sizes of integer types' of ISO/IEC 9899:TC2
Quote
The values given below shall be replaced by constant expressions suitable for use in #if
preprocessing directives. Moreover, except for CHAR_BIT and MB_LEN_MAX, the
following shall be replaced by expressions that have the same type as would an
expression that is an object of the corresponding type converted according to the integer
promotions. Their implementation-defined values shall be equal or greater in magnitude
(absolute value) to those shown, with the same sign.

Quote
minimum value for an object of type int
INT_MIN -32767 // −(215 − 1)
— maximum value for an object of type int
INT_MAX +32767 // 215 − 1

i.e. int can be anything from 16 bits up.
Title: Re: A subtle bug for you all
Post by: Flipside on October 24, 2009, 02:59:51 pm
Stuff like this is why I try to avoid re-casting in Java, with a bit of careful work, and a lot of interfaces, a lot of traps can be avoided. Java seems, to me, to be particularly prone to falling into the casting trap if you don't think about things before you start.

One of the biggest problems I ever faced was for a city-builder I was doing for my University project, I used an array of Building Object references to hold the map, and there were were multiple interfaces for buildings, such as Environment_Structure, Living_Structure etc, however, the awkward part was, you needed a seperate class for each type of building, each extending its required interface(s), (A factory would be an Industry_Structure for example, but if you wanted a live-in Factory you'd have to create a new Class that extended Living_Structure and Industry_Structure) which wasn't ideal, but did allow the game to keep simple lists of all in-game buildings that used a particular Interface automatically, and because it used interfaces, you knew the Building object you were dealing with provided certain information, it didn't matter what exact kind of structure it was :)

Edit: Also thanks to Java's lack of unsigned-byte handling, the bytemaps used to hold some data turned out to be much harder to work with than they needed to be, there was a lot of recasting byte to int and vice versa, which pleased me not a lot.

Title: Re: A subtle bug for you all
Post by: portej05 on October 25, 2009, 01:40:17 pm
OK, so you folks got number 1.
Now lets try number 2 (to which I'll post the answer in about 12 hours):

Code: [Select]
struct Data
{
  int iterations;
  int timeofday;
};

HANDLE hThread;
unsigned threadID;

unsigned __stdcall ThreadedFunc( void* p )
{
    printf("Iterations: %d\nTime of Day: %d\n", (Data*)p->iterations, (Data*)p->timeofday );
    return 0;
}

void start_thread( )
{
    Data d;
    d.iterations = 100;
    d.timeofday = 100;
    hThread = (HANDLE)_beginthreadex( NULL, 0, &ThreadedFunc, &d, 0, &threadID );
}

int main()
{
    start_thread( );
    WaitForSingleObject( hThread, INFINITE );
    CloseHandle( hThread );
}

The above code crashes randomly. Sometimes it works, sometimes it doesn't.
Why? (and yes, I found this gem in an application)
Title: Re: A subtle bug for you all
Post by: Hery on October 25, 2009, 01:58:50 pm
Pointer + stack + threads + typing code without thinking = bug
(yep, "typing code without thinking" alone also equals "bug") :P
When start_thread() returns pointer to d is no longer valid, that's not what ThreadedFunc() expects. We don't know if ThreadedFunc() is executed before or after start_thread() returns (or at the same moment if CPU count > 1), that's why it crashes randomly.
Title: Re: A subtle bug for you all
Post by: portej05 on October 26, 2009, 12:42:06 am
or Hery can post the answer 18 minutes after I post the question :P.

OK, (just to keep you on your toes) :P

Two more brief little puzzlers for you.

Example 1:
Code: [Select]
class Object
{
int value;
... functions ...
};

class Ship : public Object
{
std::string hello_world;
... functions ...
};


int main( int argc, char** argv )
{
Object* sh = new Ship( );

delete sh;
return 0;
}

The above program compiles and runs fine (just imagine a scenario where you might want to do this outside of this extremely simplified example).
But it leaks memory. A lot of memory. Why, and how do you fix it?

Example 2:
Code: [Select]
class Object
{
int value;
public:
~Object( )
{
Cleanup( );
}

void Clean( )
{
Cleanup( );
}

virtual void Cleanup( )
{
}
};

class Ship : public Object
{
void* awesome_data;
public:
virtual void Cleanup( )
{
printf("%p\n", awesome_data );
}
};


int main( int argc, char** argv )
{
Object* sh = new Ship( );
sh->Clean( );

delete sh;
return 0;
}

Questions:
1) Why does calling sh->Clean( ); print out the current value of awesome_data?
2) Given that 1) works, why doesn't Cleanup( ) get called during the destruction phase?

(If you need a hint, Pete Isensee has a fantastic presentation on this called 'The Beauty of Destructors')

EDIT: I gave the references.
Title: Re: A subtle bug for you all
Post by: Spicious on October 26, 2009, 01:14:07 am
Non-virtual destructors = fail (at least when using inheritance). More specifically for the second one, I'm going to assume that like in constructors, virtual functions are a terrible idea during destructors. It'll probably end up calling the Object version of Cleanup().
Title: Re: A subtle bug for you all
Post by: portej05 on October 26, 2009, 01:17:18 am
Spicious, that is indeed correct sir!

virtual functions are a terrible idea during destructors, even virtual destructors.
The virtual table gets destroyed pretty early in the destruction process.
Steer clear.

Destruction order is [Pete Isensee]:
1. Destructor body
2. Data members in reverse order of declaration
3. Direct non-virtual base classes in reverse order
4. Virtual base classes in reverse order

Note that this means that encapsulated objects do actually get destroyed.

Slides 26 and 27 of 'The Beauty of Destructors' are some good recommendations.
Title: Re: A subtle bug for you all
Post by: Mongoose on October 26, 2009, 01:21:30 pm
Man, now I wish I remembered anything at all from my programming structures course.  All of the class stuff apparently went in one ear and out the other.  It didn't help that the professor was terrible, I guess. :p

Just to clarify, for the first one, is the issue that you're only deleting the pointer to the ship class, and not the allocated memory itself?
Title: Re: A subtle bug for you all
Post by: portej05 on October 26, 2009, 01:27:10 pm
Just to clarify, for the first one, is the issue that you're only deleting the pointer to the ship class, and not the allocated memory itself?

No, the issue is that the destructor is non-virtual.
If the destructor was virtual, the Ship destructor would be called.

As it is, the non-virtual Object destructor is called only.
Title: Re: A subtle bug for you all
Post by: Mongoose on October 26, 2009, 01:43:50 pm
...huh.  Yeah, I'll definitely have to read that textbook again. :p
Title: Re: A subtle bug for you all
Post by: portej05 on October 29, 2009, 08:57:17 am
Pop quiz:

1) What is the insertion complexity of (and why!):
Code: [Select]
std::vector, std::list, std::map
2) Which of the following has better caching characteristics (and why!):
Code: [Select]
std::vector, std::list, std::map
3) On which of the following is random access not a O(1) operation (and why!)?
Code: [Select]
int arr[500]; std::vector, std::list
4) Given the above information, why do iterators become invalid after a non-const operation on a data structure?

5) An std::vector with 500 elements in it does not use the same amount of memory (most of the time...) as sizeof(element)*500. Why!

(A basic quiz just to keep the programming section interesting for a while :P )
Title: Re: A subtle bug for you all
Post by: Flipside on October 29, 2009, 03:21:05 pm
Well, I'd say for 3, that a list would be the answer, since, at least in my experience, lists are designed to be iterated through, not accessed non-sequentially.

True, there are situations where you can calculate the pointer position and pull directly from a list, but if you can do that, why not use an array?
Title: Re: A subtle bug for you all
Post by: Aardwolf on October 29, 2009, 05:02:18 pm
1:

vector: O(n)
list: O(1)
map: O(1)

And I'm too lazy to answer why

2:

Idunno what that means :(

3:

list, because it has to iterate through to the element every time

4:

maybe you deleted the next element, or the previous one, that an iterator pointed to? or the current? something liek that
or changed it by inserting a new element

5:

it also stores the size, and maybe some other stuff
Title: Re: A subtle bug for you all
Post by: Flipside on October 29, 2009, 06:11:36 pm
You know, thinking on the subject of Hierarchy in classes, it wonder if it reveals anything about the political position of most language designers? Or possibly the working conditions?

Look at it this way, a higher Class is completely ignorant to a connected lower Class, but that Class can do everything the higher class can do, and some more...

I know that's not related, but I'm really not certain it deserves a new thread...
Title: Re: A subtle bug for you all
Post by: Iss Mneur on October 30, 2009, 12:22:34 am
Pop quiz:
Spoiler:
To start this depends on the STL being used, STL's are not required to use the same underlying data structures to implement the data structure.

1) What is the insertion complexity of (and why!):
Code: [Select]
std::vector, std::list, std::map
Spoiler:
std::vector = O(n) - O(1) assuming that std::vector is implemented as an array and the insertion location is empty.  But normally O(n) because of the the shifting of the data.
std::list = O(n) - Assuming std::list is implemented as a linked list, O(n), because as a linked list the structure has to scan down the structure to the insertion point. Unless inserting at the start or end of list (assuming there is a tail pointer), O(1), but this is an edge case, general is still O(n).
std::map = O(1) - Assuming that std::map is implemented as a hashmap, and the hashing function properly distributes, insertions are always order O(1).
2) Which of the following has better caching characteristics (and why!):
Code: [Select]
std::vector, std::list, std::map
Spoiler:
Not clear on what you mean, but assuming you mean better for using as a caching system, it would be std::map because if it was implemented as a hashmap all accesses would be O(1).
3) On which of the following is random access not a O(1) operation (and why!)?
Code: [Select]
int arr[500]; std::vector, std::list
Spoiler:
std::list because std::list is normally implemented as a linked list and as such you have to run down the list to get your access location resulting in O(n). The array and std::vector implemented as an array would be both be just pointer dereferencing which is O(1).
4) Given the above information, why do iterators become invalid after a non-const operation on a data structure?
Spoiler:
Non-const operations can modify the data structure which may move the data around in memory causing the pointers that the iterators have to point to the wrong thing location.
5) An std::vector with 500 elements in it does not use the same amount of memory (most of the time...) as sizeof(element)*500. Why!
Spoiler:
The data structures provided by STL's normally store meta data about what is in the structure.  In std::vector's case it needs to know how large its underlying array is so that it will not go out of bound.  Also if the structure contains NULL able elements like the pointers that are allowed to be null and basic types in which 0 or some other magic value is valid, the vector then has no way of knowing how many elements are actually stored in the structure.

Not to mention that having to scan the structure to know how many elements are contained is an O(n) operation so most implemenations of std::vector store the current max size and the current number of elements in addition to the data.

You know, thinking on the subject of Hierarchy in classes, it wonder if it reveals anything about the political position of most language designers? Or possibly the working conditions?

Look at it this way, a higher Class is completely ignorant to a connected lower Class, but that Class can do everything the higher class can do, and some more...

I know that's not related, but I'm really not certain it deserves a new thread...

I think you are thinking into this too much.  The real problem is that scanning the function tables of all derived classes is an expensive operation so C++ does not do this by default but allows the programmer to use the 'virtual' keyword to selectively turn on this behavior. 
Title: Re: A subtle bug for you all
Post by: portej05 on October 30, 2009, 01:22:06 am
IssMneur: ISO/IEC 14882:2003(E) specifies (page 489)
Quote
The elements of a vector are stored contiguously, meaning that if v is a vector<T, Allocator> where T is some type other than bool, then it obeys the identity &v[n] == &v[0] + n for all 0 <= n < v.size().

So I should have said any conforming implementation :P

You might be surprised to learn that a vector supports amortised constant time inserts and erases at the end:
Quote
In addition, it supports (amortized) constant time insert and erase operations at the end; insert and erase in the middle take linear time.

(we'll come back to this for question 5)
I'm pretty sure IssMneur nailed it for question 1, except that a vector is 'normally' constant time (according to the standard).

2) By this I meant for regularly accessed data in a tight loop. Vector has significantly better data locality, so will perform significantly better than other data structures where a caching system is provided that is significantly faster than accesses into main memory.

3) No surprises - it's the list!

4) [quote IssMneur]
Non-const operations can modify the data structure which may move the data around in memory causing the pointers that the iterators have to point to the wrong thing location.[/quote]

5) This one you're both partially correct, but it's only a small overhead required for housekeeping.
Going back to question 1, the insert time for a vector is amortised constant.
This is achieved by OVERALLOCATING each time a reallocation and move is done.
So if you're pushing a 100 1KB objects onto a vector, you might be surprised to learn that the final vector size might be something like 150 elements (again, depends on the implementation).
Write yourself a little program comparing the return values from vector::capacity and vector::size and you'll see what I mean here.

I'm going to have to think to come up with something more challenging for you folks!
Title: Re: A subtle bug for you all
Post by: Flipside on October 30, 2009, 04:14:22 am
You know, thinking on the subject of Hierarchy in classes, it wonder if it reveals anything about the political position of most language designers? Or possibly the working conditions?

Look at it this way, a higher Class is completely ignorant to a connected lower Class, but that Class can do everything the higher class can do, and some more...

I know that's not related, but I'm really not certain it deserves a new thread...

I think you are thinking into this too much.  The real problem is that scanning the function tables of all derived classes is an expensive operation so C++ does not do this by default but allows the programmer to use the 'virtual' keyword to selectively turn on this behavior.  

To be honest, that was intended more as a humorous observation than a genuine attempt to look into the mentality behind language design ;) But, admittedly, I probably should have made that clear :)
Title: Re: A subtle bug for you all
Post by: Iss Mneur on October 30, 2009, 01:02:41 pm
You know, thinking on the subject of Hierarchy in classes, it wonder if it reveals anything about the political position of most language designers? Or possibly the working conditions?

Look at it this way, a higher Class is completely ignorant to a connected lower Class, but that Class can do everything the higher class can do, and some more...

I know that's not related, but I'm really not certain it deserves a new thread...

I think you are thinking into this too much.  The real problem is that scanning the function tables of all derived classes is an expensive operation so C++ does not do this by default but allows the programmer to use the 'virtual' keyword to selectively turn on this behavior.  

To be honest, that was intended more as a humorous observation than a genuine attempt to look into the mentality behind language design ;) But, admittedly, I probably should have made that clear :)
I wasn't sure if you were being serious or not, but because that was not the first time I had seen that particular comment (not from you, just in general) I just figured that I would clarify it for everyone.

portej05: I know some of this we already talked about last night on IRC, but I figured I should clarify a few things.
IssMneur: ISO/IEC 14882:2003(E) specifies (page 489)
Quote
The elements of a vector are stored contiguously, meaning that if v is a vector<T, Allocator> where T is some type other than bool, then it obeys the identity &v[n] == &v[0] + n for all 0 <= n < v.size().

So I should have said any conforming implementation :P

You might be surprised to learn that a vector supports amortised constant time inserts and erases at the end:
Quote
In addition, it supports (amortized) constant time insert and erase operations at the end; insert and erase in the middle take linear time.

(we'll come back to this for question 5)
I'm pretty sure IssMneur nailed it for question 1, except that a vector is 'normally' constant time (according to the standard).
Like I said at the top of my reply, it depends on the STL that you are using.

To be honest, I have never read the C++ specs and wasn't aware that the specs even specified what the STL was required to do except possibly for the interface.  I actually thought the STL was just a defacto standard and not formalized by the ISO, based on the STL's history.

That being said, because you didn't say where the insertions are, you cannot depend on the amortized cost because it only comes into effect when appending to the end of the vector. Which as I noted I my response: "O(1) assuming that std::vector is implemented as an array and the insertion location is empty.  But normally O(n) because of the the shifting of the data." I was aware of the amortization which is what I was trying to get at when I said the insertion location is empty.  Insertion location can be empty for two reasons, over allocation or gaps in the vectors array; though only over allocation will cause insertion location to be empty when using the insert() call.  By gaps I mean you the coder know that you can just overwrite a specific location because you made what was there invalid in some way; that is, just setting at an index vector[index] = something.

2) By this I meant for regularly accessed data in a tight loop. Vector has significantly better data locality, so will perform significantly better than other data structures where a caching system is provided that is significantly faster than accesses into main memory.
The CPU's cache! I was definitely not expecting you to be talking about that level, but as I said in my response, I was not clear as to what you were getting at, as caching can mean a lot of things and levels.

5) This one you're both partially correct, but it's only a small overhead required for housekeeping.
Going back to question 1, the insert time for a vector is amortised constant.
This is achieved by OVERALLOCATING each time a reallocation and move is done.
So if you're pushing a 100 1KB objects onto a vector, you might be surprised to learn that the final vector size might be something like 150 elements (again, depends on the implementation).
Write yourself a little program comparing the return values from vector::capacity and vector::size and you'll see what I mean here.
Okay, I took it at face value, that is, why is the memory usage of 500 elements in an array and 500 elements in std::vector not exactly equal.
Title: Re: A subtle bug for you all
Post by: portej05 on October 30, 2009, 11:46:16 pm
I really should have worded those much better than I did....
Title: Re: A subtle bug for you all
Post by: portej05 on November 03, 2009, 09:00:47 am
A new (very well specified) one for you all (and mostly from the engine :D ):

Code: [Select]
typedef struct color {
uint screen_sig;
ubyte red;
ubyte green;
ubyte blue;
ubyte alpha;
ubyte ac_type; // The type of alphacolor.  See AC_TYPE_??? defines
int is_alphacolor;
ubyte raw8;
int alphacolor;
int magic;
} color;

Here's the question:
Code: [Select]
sizeof( color ) == x

What is x, and why.
Title: Re: A subtle bug for you all
Post by: Hery on November 03, 2009, 09:22:21 am
28 (at least on x86)
Data structure alignment requires address of variable to be a multiplicity of its size. That's why when we have, for example:
Code: [Select]
struct foo {
    char one;
    int two;
    char three;
};

struct bar {
    char one;
    char two;
    int three;
};
foo size is 12 bytes while bar takes 8 bytes of memory. If we really need data to be packed one directly after another then we can use implementation defined directives (that's useful while dealing with low-level stuff).
The whole data alignment stuff depends on the implementation and is not a thing we should rely on (unless using kind of packed attribute or using only x86).
Title: Re: A subtle bug for you all
Post by: portej05 on November 13, 2009, 07:58:34 am
OK, here's another (from line 342 of ddsutils.cpp):

Code: [Select]
char real_filename[MAX_FILENAME_LEN];
...
memset( &real_filename, 0, sizeof(real_filename) );

What's wrong with it, and would it work anyway?
Title: Re: A subtle bug for you all
Post by: Uchuujinsan on November 13, 2009, 08:44:19 am
Code: [Select]
char real_filename[MAX_FILENAME_LEN];
...
memset( real_filename, 0, sizeof(char)*MAX_FILENAME_LEN );
That's the way it should work, shouldn't it?

Code: [Select]
memset( &real_filename, 0, sizeof(real_filename) );
Should be similar to
Code: [Select]
real_filename = 0;
Title: Re: A subtle bug for you all
Post by: Hery on November 13, 2009, 11:17:17 am
Code: [Select]
char real_filename[MAX_FILENAME_LEN];
...
memset( &real_filename, 0, sizeof(real_filename) );

What's wrong with it, and would it work anyway?
I was cheating a bit, because I had to check few things in the debugger (and I'm still not sure about the whole thing, though).
Apparently, the reference operator is redundant but is is not an error. "real_filename" is a name of an array of char, which can be converted to a pointer. It can be implicit or explicit, using reference operator. That's why real_filename == &real_filename.

Uchuujinsan: sizeof(char) is redundant since it by definition equals 1.
Title: Re: A subtle bug for you all
Post by: portej05 on November 13, 2009, 09:51:16 pm
I'm sticking this here to remind me to go back and do it properly when we get a better memory management solution :P

Line 1053 of modelread.cpp
Code: [Select]
for ( i = 0; i < pm->n_models; i++ )
{
  /* HACK: This is an almighty hack because it is late at night and I don't want to screw up a vm_free */
  new ( &( pm->submodel[ i ].buffer ) ) SCP_vector<buffer_data>( );
  pm->submodel[ i ].Reset( );
}
Title: Re: A subtle bug for you all
Post by: Uchuujinsan on November 13, 2009, 10:42:49 pm
Hery:
I know, thx, I just wrote it there to make the structure clearer. The compiler would remove it anyway, so it has no runtime influence.
Title: Re: A subtle bug for you all
Post by: Spicious on November 13, 2009, 11:31:24 pm
If it didn't automatically convert the array to a pointer it wouldn't compile. Arrays are like literals; they don't have addresses in that sense. real_filename = 0 would not compile for that reason. In terms of consistency it's still horrible though. Using sizeof(real_filename) is fine as long as it's an array and not a pointer.
Title: Re: A subtle bug for you all
Post by: Uchuujinsan on November 14, 2009, 01:17:12 am
Hm, interesting, didn't know that :>
Title: Re: A subtle bug for you all
Post by: Hery on November 14, 2009, 04:30:39 am
Spicious: Arrays are nonmodifiable lvalues (thus they have addresses) while literals are always rvalues.
Title: Re: A subtle bug for you all
Post by: portej05 on November 14, 2009, 10:57:03 pm
Here's one from all over the engine (a simplified case):
Code: [Select]
struct SomeStruct
{
  int x;
  SomeStruct( )
       : x(10)
  {
  }
};

SomeStruct* ss = vm_malloc( sizeof(SomeStruct) );

What is the value of 'x' when accessed via ss->x after the call to vm_malloc, why, and if there is a problem, what code should be changed?
Title: Re: A subtle bug for you all
Post by: Uchuujinsan on November 15, 2009, 05:44:42 am
I'm not entire sure what vm_malloc does, but I would guess it doesn't call the constructor, so ss->x should be 0 in debug mode, and unitialized/random in release.
Should be fixed by using "new" instead.
Maybe I guess right this time :>
Title: Re: A subtle bug for you all
Post by: portej05 on November 15, 2009, 06:18:25 am
Indeed, you are correct.
The value of x is actually undefined though in both release and debug modes :)
Visual Studio debug CRT will initialise it to 0xcdcdcdcd (ever wondered why malloc is so slow in debug modes!)
New is the solution to the problem.
You can either do placement new (i.e. new ( ss ) SomeStruct( ) ) or just replace the vm_malloc with new SomeStruct( );
Title: Re: A subtle bug for you all
Post by: Uchuujinsan on November 15, 2009, 10:40:09 am
Ah ok, I've only used VS yet, and I knew that debug mode always initilizes to a fixed value while release does not.
Title: Re: A subtle bug for you all
Post by: portej05 on December 13, 2009, 07:30:11 am
OK, programming question time (from parselo.cpp):

What is the reason that you need const_cast?
Code: [Select]
bool can_construe_as_integer(const char *text)
{
// trivial case; evaluates to 0
if (*text == '\0')
return true;

// number sign or digit for first char
if ((*text != '+') && (*text != '-') && !isdigit(*text))
return false;

// check digits for rest
// (why on earth do we need a const cast here?  text isn't the pointer being modified!)
for (char *p = const_cast<char*>(text) + 1; *p != '\0'; p++)
{
if (!isdigit(*p))
return false;
}

return true;
}
Title: Re: A subtle bug for you all
Post by: Aardwolf on December 13, 2009, 02:22:26 pm
OK, programming question time (from parselo.cpp):

What is the reason that you need const_cast?

*snip*


The answer is obvious: code gremlins. If you don't const_cast, they will sabotage your code at runtime and cause you to crash.
Title: Re: A subtle bug for you all
Post by: Uchuujinsan on December 13, 2009, 05:29:57 pm
isdigit(char*) instead of isdigit(const char*)?
Title: Re: A subtle bug for you all
Post by: Hery on December 13, 2009, 06:03:59 pm
Uchuujinsan: isdigit is not interested in pointers, it wants values ;p

In this part of code we can easily get rid of const_cast by making p const char*. We still would be able to increment p value:
const char * p; <- pointer to constant data
char * const p; <- constant pointer to data
Title: Re: A subtle bug for you all
Post by: Uchuujinsan on December 13, 2009, 07:32:28 pm
ah, damn, didn't look close enough :>
Title: Re: A subtle bug for you all
Post by: portej05 on December 13, 2009, 10:16:47 pm
Hery: Yep!

The issue becomes what happens when the compiler doesn't think that the value of 'text' is going to get changed in the function (remember, it's 'const'), so if you use const_cast weird stuff can happen when the compiler optimises your code.
Title: Re: A subtle bug for you all
Post by: portej05 on January 29, 2010, 09:56:29 am
Happy New Year all, and I've found a brief doozy for you all.

Code: [Select]
class Interface1
{
public:
virtual void Int11( ) = 0;
virtual void Int12( ) = 0;
};

class Interface2
{
public:
virtual void Int21( ) = 0;
virtual void Int22( ) = 0;
};

class MI : public Interface1, public Interface2
{
public:
virtual void Int11( ) { return; }
virtual void Int12( ) { return; }
virtual void Int21( ) { return; }
virtual void Int22( ) { return; }
};

int main( int argc, char** argv )
{
MI* i = new MI( );
Interface1* b = dynamic_cast< Interface1* >( i );
delete b;

MI* j = new MI( );
Interface2* c = dynamic_cast< Interface2* >( j );
delete c;

return 0;
}

Where and why does the above code crash (tested with MSVC2008) (Hery is away until Feb8, giving you folks a head start :P ).

Hint: Understanding what dynamic_cast is/does (although in this case, it could be substituted with static_cast) may help.
Title: Re: A subtle bug for you all
Post by: Aardwolf on January 30, 2010, 09:06:57 pm
I'm gonna go with "delete b;", as you've got virtual functions but no virtual destructor.
Title: Re: A subtle bug for you all
Post by: portej05 on January 31, 2010, 12:02:18 am
It's actually a bit more complex than that :)
Think about what dynamic_cast does, and what the values of b and c might be.

There is no issue with 'delete b', that line runs correctly (amazingly - there is a fair bit wrong with that code, but it crashes in one particular place) :)
Title: Re: A subtle bug for you all
Post by: Aardwolf on January 31, 2010, 02:03:46 am
The return statement, then. Something about the local variable pointers, I think.
Title: Re: A subtle bug for you all
Post by: Bobboau on January 31, 2010, 03:13:21 am
I'm thinking it would crash as it was shutting down, not really at the return but sometime after it, when the OS frees all the memory, it will try to free what i and j were pointing at (and leaked), part of the memory has already been freed, so it will have a 'trying to free memory you didn't allocate' error.
Title: Re: A subtle bug for you all
Post by: portej05 on January 31, 2010, 09:23:16 am
Sorry guys, not quite.
Read my previous hint :)
Then think about the implementation of 'delete'.

BTW, as far as I can tell (reading the standard and experimenting a little), all destructors run correctly whether delete c or delete b.

Another hint: The crash occurs at 'delete c;'
Title: Re: A subtle bug for you all
Post by: Uchuujinsan on January 31, 2010, 01:09:14 pm
Spoiler, because I cheated and used a compiler (and still didn't get it)
Spoiler:
If you make a virtual distructor for Interface2, it won't crash.

Found a little something about that matter:
http://stackoverflow.com/questions/294927/does-delete-work-with-pointers-to-base-class

Well, i == b (the address)
while j== b+2(!) (again the address), because Interface2 is placed later in the memory. That's why switching Interface1 and Interface2 in the inheritance will crash the programm at delete b.

Why exactly this all happens... don't really know, I just know I wouldn't write something like this :>
But certainly a "beautifull" bug :)
Title: Re: A subtle bug for you all
Post by: portej05 on March 07, 2010, 06:39:21 pm
Today features a subtle bug from Wanderer:

Code: [Select]
cg->moveflag_dest = HUD_VAR(custom_gauge_moveflags[0]) + (Num_custom_gauges * sizeof(bool));

Your task:
How many bugs can you spot?

NB: HUD_VAR is a macro resolving to offsetof
Title: Re: A subtle bug for you all
Post by: Mika on March 08, 2010, 01:13:48 pm
Today features a subtle bug from Wanderer:

Code: [Select]
cg->moveflag_dest = HUD_VAR(custom_gauge_moveflags[0]) + (Num_custom_gauges * sizeof(bool));

Your task:
How many bugs can you spot?

NB: HUD_VAR is a macro resolving to offsetof

Uh-oh

Even at the risk of making myself look like a donkey, I think there are at least three.

Could you post the assigned datatypes of this snippet? What is custom_gauge_moveflags supposed to be? A boolean? I have never seen anything related to FSOpen, and can't bother to download the thing yet to check it for myself.

My guess is that this is written C++? I don't remember the boolean definition in it, but looks like if moveflag_dest is boolean (one bit for a yes/no flag?), the result of the right hand side can become something else.

Num_custom_gauges is probably integer and may become greater than 1, yet it multiplied by sizeof(bool)? Thus resulting in integer/garbage?

HUD_VAR is a macro operating to a vector (or worse, a pointer)? I don't know exactly if this is an error here, but generally macros tend to result in disasters if coder isn't exactly sure how C expands them. Especially if it has something like SOME_MACRO( int c + int b)...

ADDENDUM:
What is Num_custom_gauges * sizeof(bool) even supposed to do?
Title: Re: A subtle bug for you all
Post by: portej05 on March 09, 2010, 01:06:55 am
I'm not sure of the entire context myself - assume that custom_gauge_moveflags is a boolean array for now.
There is a bigger problem though: What is sizeof(bool)?
Title: Re: A subtle bug for you all
Post by: sigtau on March 10, 2010, 05:48:25 pm
The bit width of a certain data type, I would assume.  Booleans are raw binary values--that is to say, they are literally true or false--one or zero.

At least, I'm just taking a shot in the dark here.  That's how it works in Java, last I checked.
Title: Re: A subtle bug for you all
Post by: portej05 on March 11, 2010, 12:59:42 am
To be honest, I'm still trying to figure out if there's a problem there, but the issue I can see is that sizeof(boolean) is implementation dependent.
In addition, I believe the return value from offsetof is a size_t which causes the sizeof bool to become important.
I haven't coded up an example for this one, so would be happy to be shown either way.
Title: Re: A subtle bug for you all
Post by: Bobboau on March 14, 2010, 06:06:09 am
sizeof(bool) == 4

yes it is that bad.
Title: Re: A subtle bug for you all
Post by: portej05 on March 14, 2010, 06:15:02 am
sizeof(bool) == 1 on my machine :P
Title: Re: A subtle bug for you all
Post by: Bobboau on March 14, 2010, 07:44:40 am
rely? you've checked?what platform/build environment are you using?
Title: Re: A subtle bug for you all
Post by: portej05 on March 15, 2010, 02:00:06 am
VS2008 SP1 Win7 32bit C++
Title: Re: A subtle bug for you all
Post by: Nuke on March 16, 2010, 12:28:19 am
id have said 2 myself, but im stupid. this whole thread is way ove rmy head, il just stick to lua and other interpreted languages :D