Solved

VC++ Compiler Optimization Options

Posted on 2000-03-03
28
437 Views
Last Modified: 2010-05-18
Hi.
I've written an ATL DLL that uses MFC and the CRT.
It works fine in Debug builds, but crashes when it runs in Release builds.
I've noticed, after many hours of disassembly debugging, that if I disable optimization the problem is fixed.
The 'problem' is an access vilation exception. In my code I'm allocating memory into a buffer and trying to access the [0] item - and getting an exception. I've checked and malloc seemed to work.
Any ideas?
0
Comment
Question by:AssafLavie
  • 9
  • 5
  • 5
  • +2
28 Comments
 
LVL 32

Expert Comment

by:jhance
Comment Utility
In my experience, when Debug works and Release fails it's because you are overwriting a memory location that is "padded" in the debug build but not in the release build.  

Usually you will get a report on this if you run the debug build in the debugger.  The debug runtime library is designed to try and catch these types of errors.

If not, check the code where the error is happening in the release build and use the linker's MAP file (you may need to enable this output as it's not the default) to find the code where it's happening.  Then find this code in the COD file and see what's going on there.

Go back to your debug build and try and find where you are stomping on this memory.
0
 
LVL 4

Author Comment

by:AssafLavie
Comment Utility
Thanks jhance, But:
1. The Debug versions works well outside the debugger _as well_ as inside the debuger.
2. The code that causes the excpetion is relativly small and pretty straight forward. I use malloc to allocate an array of unsigned chars and than I try write into the first item.
Of course, I'm checking for NULLs etc.

The reason I know exactly where the exception occurs is that I tried changing the settings of the Release build to include debug information. Of course I still got the exception - but then at least I was able to see the code that crashed. Later I found out that is was the optimization flag that brought my program down.
0
 
LVL 22

Expert Comment

by:nietod
Comment Utility
You are not telling us everything because you can be sure that the optimizer does not make such simple code crash.  So there is something that you are doing that causes the optimized code to crash, but what that is we can't say.  

For instance, one thing I've seen is the in non-optimized code you can "safely" return refereces to local varaibles.  This is of course a mistake, but it works in vc (5 at least) in the debug version, but not in the release.

If you post the code we might be able to spot the problem.
0
 
LVL 32

Expert Comment

by:jhance
Comment Utility
It's easy to say that there must be a bug in the compiler, and I've been there myself.  Unfortunately, it's ALWAYS BEEN MY MISTAKE, regardless of what I originally thought.

I'm a firm believer that the simplest answer is almost always the right one.  The fact is that you're making a programming error that is being masked by the debug build.

Good luck on solving this, however.  I question whether you are able to find it since you seem so certain that it's not your fault!

Since you can't seem to post any of your offending code, I'm assuming you have something like:

      unsigned char *pUCHAR;

      pUCHAR = new unsigned char [32];

      for(int i = 0; i<32; i++){
            pUCHAR[i] = 'a';
      }


      delete [] pUCHAR;

It works for me....
0
 
LVL 4

Author Comment

by:AssafLavie
Comment Utility
You're got me, jhance, I was trying to blame the compiler for my bug - you saw right through me!
<wink>
Dont get me wrong, I really do appreciate your help. But your answer was:
1. "It doesn't crash in Debug cause the debugger is catching your exceptions."
2. "You should find the bug."
And it didn't help me too much.

I apologize for not publishing the code though, I didn't think it was necessary at the time.
Here it is: (This function converts a string of 2 digit hexadecimal numbers into an array of unsigned chars (half the size))

typedef BUFFER unsigned char *
HRESULT CMyClass::Hex2Bin(const char *      szHexadecimal,      // [in]
                                      BUFFER&            lpBinary,            // [out] (Freed by caller)
                                      int&                  nBufferLen)            // [out]
{
      // Allocate half of the string's length.
      nBufferLen = strlen(szHexadecimal) / 2;
      lpBinary = (BUFFER)malloc(nBufferLen * sizeof(unsigned char));
      if (lpBinary == NULL)
            return E_OUTOFMEMORY;

      for (int i = 0; i < nBufferLen; i++)
      {
            char szTemp[3] = {0};
            // Read the next two chars to a temporary string.
            strncpy(szTemp, &szHexadecimal[i * 2], 2);

            // Convert the tiny string to an integer.
            unsigned char nTemp = 0;
            sscanf(szTemp, "%X", &nTemp);
            lpBinary[i] = nTemp;
      }
      return S_OK;
}

By the way, If anyone has a better way to convert hexa strings to integer I'd love to
hear it.

Thanks again,
Assaf.
0
 
LVL 32

Expert Comment

by:jhance
Comment Utility
You have:

typedef BUFFER unsigned char *

and:

lpBinary = (BUFFER)malloc(nBufferLen * sizeof(unsigned char));


This allocates "unsigned char *" not "unsigned char".  You are then storing:

lpBinary[i] = nTemp;

But this is storing into the wrong place.  You're referring to the "i"th "unsigned char *" and not the "i"th character of the string.

I think you're mixing up your pointers and pointed to objects.

0
 
LVL 4

Expert Comment

by:abancroft
Comment Utility
You can use sscanf. e.g.

const char *szHex="0x0100";
int nHex;
sscanf(szHex, "%x", &nHex);

You can supply other format specifiers to tailor the conversion.
0
 
LVL 22

Expert Comment

by:nietod
Comment Utility
As jhance said you are using malloc() wrong, but the real question is why are you using it in the first place?  You should be using new.
0
 
LVL 4

Expert Comment

by:abancroft
Comment Utility
I must be missing something.

I thought malloc(nBufferLen * sizeof(unsigned char)) would allocate a memory block of size (nBufferLen * sizeof(unsigned char)) and then return a void pointer.

AssafLavie just converts the void* to unsigned char*.

Where does he allocate unsigned char*?

As I was taught, in C an array reference is the same as a pointer to the first array element.

e.g.
unsigned char a_uchar[32];
BUFFER pArray = a_uchar;

a_uchar[0]='a';

_assert(a_uchar[0]==pArray[0]);

Isn't this correct?

0
 
LVL 32

Expert Comment

by:jhance
Comment Utility
>AssafLavie just converts the void* to
>unsigned char*.

>Where does he allocate unsigned char*?


That's the whole problem.

He's allocating an array of "unsigned char *" instead of an array of "unsigned char".  In the debug build, this is working through some fluke of the way memory is initialized in the debug library.  In the release build, it's an invalid memory reference.
0
 
LVL 4

Expert Comment

by:abancroft
Comment Utility
I still don't see a problem.

What's the difference between his code and:
  unsigned char *lpBinary;

  lpBinary = (unsigned char *)malloc(64 * sizeof(unsigned char));

  lpBinary[0] = 'a';

  free(lpBinary);
0
 
LVL 4

Expert Comment

by:abancroft
Comment Utility
Or the equivalent C++:
  unsigned char *lpBinary = new unsigned char[64];

  lpBinary[0] = 'a';

  delete []lpBinary;
0
 
LVL 32

Expert Comment

by:jhance
Comment Utility
Now I'm mixed up again on this too...

I'll have to stare at this code some more now.
0
 
LVL 22

Expert Comment

by:nietod
Comment Utility
one problem is

strncpy(szTemp, &szHexadecimal[i * 2], 2);

if th strng to be copied is length 2 or greater, then the desitnation string (szTemp) is not terminated.  

You need to manially set the last character of szTemp to 0 like

szTemp[2] = 0;
0
Do You Know the 4 Main Threat Actor Types?

Do you know the main threat actor types? Most attackers fall into one of four categories, each with their own favored tactics, techniques, and procedures.

 
LVL 22

Expert Comment

by:nietod
Comment Utility
>> If anyone has a better way to convert hexa strings
>> to integer I'd love to hear it

int HexDig2Int(char c)
{
   c = toupper(c);
   if (c >= 'A')
      c -= ('A'-10);
   else
     c -=  '0';
   return c;
}

to convert a pair of digits you would do

int x = 4*HexDig2Int(FirstChar) + HedDig2Int(SecondChar);

That would simplify you code.  You  might need to add an additional test if the leading digit can be a space.
0
 
LVL 11

Expert Comment

by:mikeblas
Comment Utility
nietod> You need to manially set the last character of
 nietod> szTemp to 0 like szTemp[2] = 0;

No.  

   char szTemp[3] = {0};

has already done that.

..B ekiM
0
 
LVL 22

Expert Comment

by:nietod
Comment Utility
Why?

its not a global so it won't be zero inited.  When initializing an aggregate:

2 When  an  aggregate (whether class or array) contains members of class
  type  and  is  initialized  by   a   brace-enclosed   initializer-list
  (_dcl.init.aggr_),   each   such   member   is  copy-initialized  (see
  _dcl.init_) by the corresponding assignment-expression.  If there  are
  fewer  initializers in the initializer-list than members of the aggre-
  gate, each member not explicitly initialized shall be copy-initialized
  (_dcl.init_)  with  an initializer of the form T() (_expr.type.conv_),
  where T represents the  type  of  the  uninitialized  member.   [Note:
  _dcl.init.aggr_  describes  how  assignment-expressions in an initial-
  izer-list are paired with the aggregate members  they  initialize.   ]

And T() for a char does nothing.
0
 
LVL 11

Expert Comment

by:mikeblas
Comment Utility
> Why?  

Do you really want me to explain it?  If so, open a question with some points.

 > T() for a char does nothing.

Uh... no.

..B ekiM
0
 
LVL 11

Expert Comment

by:mikeblas
Comment Utility
<sigh> Screw it: I'll be generous.

Quoting from the FIDS:

8.5.1.7: If there are fewer initializers in the list than there are members in the aggregate, then each member not explicitly initialized shall be default-intialized (8.5).

That's certainly the case here; we've only supplied on initializer (zero) and the rest will have to be default initialized.  Default initialized means:

8.5: To default-initialize an object of type T means:

  - if T is a non-POD class type (clause 9), the default constructor for T is called (and the initialization is ill-formed if T has no accessible default constructor);

  - if T is an array type, each element is default initialized;

  - otherwise, the storage for the object is zero-initialized.

Now, you should see.

..B ekiM

0
 
LVL 22

Expert Comment

by:nietod
Comment Utility
I wasn't aware that the default initializer for POD is 0. But please, don't ever go out of your way on my account.
0
 
LVL 4

Author Comment

by:AssafLavie
Comment Utility
Sorry for being away for while after posting my question.

So everybody agrees that malloc was called correctly?
Nietod, Jhance: I really don't think that typecasting malloc's return value changes the type of my array's members. Does it?

And everybody agrees that the temporary string is initialized to 0's (NULL terminated)?

So where's the bug? sscanf? I think I'm using it correctly.


0
 
LVL 22

Accepted Solution

by:
nietod earned 100 total points
Comment Utility
>> everybody agrees that malloc was called correctly?
No.  It is hard to say.  You don't indicate what data type lpbuffer points to.  It is declared as

BUFFER& lpBinary

if BUFFER is "char *" then you are allocating just enough space to store the data (and no terminator, if you woudl want one).  if it is delcated as "int *" you are not allocating enough space.  We can't tell.  That is one reason why you shoudln't be using malloc()

>> I really don't think that typecasting
>> malloc's return value changes the type
>> of my array's members
No but it allows you to "cover up" mistakes the compiler would be able to catch if you use new.  I can't think of any reason to use malloc in a c++ program unless you need to maintain C compatiblity (or when overloading a new operator.)

>> And everybody agrees that the temporary
>> string is initialized to 0's (NULL terminated)?
It appears that it is.  But I would not try to write code that relies on those sort of subtleties.  Its likely to be missundestood.  Code that clearly shows the array being initalized might be better, like

char szTemp[3] = {0,0,0};

or

char szTemp[3];
szTemp[2] = 0;

Since you are always copying exactly 2 bytes and never a NUL terminator you could have used memcpy instead of strncpy() (or just move the characters individually).


There is a bug here

unsigned char nTemp = 0;
sscanf(szTemp, "%X", &nTemp);

you must pass an int, not a char for a %X type value.  howeve th code I suggested instead will be more efficient.
0
 
LVL 11

Expert Comment

by:mikeblas
Comment Utility
nietod> You don't indicate what data type lpbuffer points to

The first line of the code AssafLavie posted shows the answer:

   typedef BUFFER unsigned char *;

 nietod> But I would not try to write code that relies on those sort of subtleties.

I'm not sure that's a subltety. If you're initializing an object, that syntax is the only reliable way to quickly initialize all member variables to zero.

 nietod> But please, don't ever go out of your way on my account.

What choice am I left with when you driectly ask me?

..B ekiM
0
 
LVL 22

Expert Comment

by:nietod
Comment Utility
You can not answer.  I don't want to put you out.  
0
 
LVL 4

Author Comment

by:AssafLavie
Comment Utility
Well I got rid of sscanf and it seems that it was the source of all my trouble after all. Instead, I used Nietod's hex2dec function (Hope you don't mind).
I believe the problem was in the use of sscanf with a char and not an int, as Nietod suggested.
I'd like to give Nietod the points even though he/she did not propose an 'answer' (only a comment).
(I hope that's fine with everybody).

Thank you all.
0
 
LVL 4

Author Comment

by:AssafLavie
Comment Utility
Thanks everybody.
0
 
LVL 11

Expert Comment

by:mikeblas
Comment Utility
AssafLavie> I believe the problem was in the use of sscanf with a char and not an int

Indeed, this is correct. The reason it only shows in a release build is that the compiler is more agressive about packing variables on the stack when optimizations are on. The overwrite fell into padding space in a debug build--but into a used variable (or the stack frame for the function!) in a debug build.

..B ekiM
0
 
LVL 22

Expert Comment

by:nietod
Comment Utility
>> (Hope you don't mind).
If it was a secret, I wouldn't post it.  :-)
0

Featured Post

How your wiki can always stay up-to-date

Quip doubles as a “living” wiki and a project management tool that evolves with your organization. As you finish projects in Quip, the work remains, easily accessible to all team members, new and old.
- Increase transparency
- Onboard new hires faster
- Access from mobile/offline

Join & Write a Comment

Introduction This article is the first in a series of articles about the C/C++ Visual Studio Express debugger.  It provides a quick start guide in using the debugger. Part 2 focuses on additional topics in breakpoints.  Lastly, Part 3 focuses on th…
This article will show you some of the more useful Standard Template Library (STL) algorithms through the use of working examples.  You will learn about how these algorithms fit into the STL architecture, how they work with STL containers, and why t…
The viewer will learn how to pass data into a function in C++. This is one step further in using functions. Instead of only printing text onto the console, the function will be able to perform calculations with argumentents given by the user.
The viewer will be introduced to the member functions push_back and pop_back of the vector class. The video will teach the difference between the two as well as how to use each one along with its functionality.

743 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

16 Experts available now in Live!

Get 1:1 Help Now