Why are these 2 code blocks not the same?

These 2 code blocks should function identically, I would think.

I marked in bold the only thing I changed (2 places).

This code functions properly:

Code:

---

#define MAX_SECTORS_PER_READ    27
#define RAW_SECTOR_SIZE    2352

ULONG  sectorsPerRead;
ULONG  bytesPerRead;

sectorsPerRead = MAX_SECTORS_PER_READ;
bytesPerRead = sectorsPerRead * RAW_SECTOR_SIZE;

fSuccess = DeviceIoControl(pextr->hCDROM,
                          IOCTL_CDROM_RAW_READ, 
                          &info,
                          sizeof(RAW_READ_INFO), 
                          smpl,
                          sectorsPerRead * RAW_SECTOR_SIZE,     
                          &bytesRead,           
                          0);                   

if(bytesRead != sectorsPerRead * RAW_SECTOR_SIZE) __leave;

---

This one fails:

Code:

---

#define SECTORS_PER_READ    27
#define RAW_SECTOR_SIZE    2352

ULONG  sectorsPerRead;
ULONG  bytesPerRead;

sectorsPerRead = MAX_SECTORS_PER_READ;
bytesPerRead = sectorsPerRead * RAW_SECTOR_SIZE;
                 
fSuccess = DeviceIoControl(pextr->hCDROM,       
                          IOCTL_CDROM_RAW_READ, 
                          &info,                 
                          sizeof(RAW_READ_INFO),
                          smpl,                 
                          bytesPerRead,         
                          &bytesRead,           
                          0);                 

if(bytesRead != bytesPerRead) __leave;

---

These makes no sense to me, as I see them as identical.

Am I missing something here?

Use your debugger, is the value of "bytesread" the same in both cases?

I don't know how to use the debugger. :blush:

Quote:

---

Originally Posted by mmscg

I don't know how to use the debugger. :blush:

---

Well you need to learn to use it for cases such as yours. No one will be able to duplicate this problem, since the two blocks seem identical.

But to let you know, you won't get far at all in this field unless you know how to debug your own programs using the debugger. It is a requirement, so you need to learn it.

What if your program were 10,000 lines long? How would you go about solving the problem? I don't think posting such a program on CodeGuru will get you a lot of help.

Regards,

Paul McKenzie

You could convert bytesPerRead and the other to a string and then show what their values are in a messagebox.

Look closely... :ehh: The first two lines differ. I'm not sure if this is a typo but if not then you might have defined MAX_SECTORS_PER_READ somewhere else with a different value.

Paul McKenzie
No instructions came with VC++ that I purchased, nothing about debugger that I've ever seen on the net, ditto in any C++ books I bought.

If you'd care to explain, I'll listen.


aewarnick
That's what I am doing right now...


angelorohit
Should both be MAX_SECTORS_PER_READ (OK in my program, typo here at forum)
Good catch though!!

Well you haven't searched around much on the net then since in less than 1 minute I've found loads of resources on debugging.

Try this one for starters

Visual Studio Debugging

As Paul said you won't get far if you can't use a debugger.

Regards
Alan

OK thanks! (I guess I wasn't searching properly... C++ tutorial, win32 tutorial, etc. were typical searches I did)

OK, after learing how to set breakpoints while using the debugger, I have determined why
the two code blocks do not behave the same.

Quote:

---

Use your debugger, is the value of "bytesread" the same in both cases?

---

Yes, they are the same in both cases.

However, the value of sectorsPerRead CHANGES during the execution of a loop,
even though I do not reset it's value.

This should be impossible, should it not?

Here is the loop:

Code:

---

do
{
    while(startingSector + sectorsPerRead <= endingSector)
    { 

        info.DiskOffset.QuadPart = (ULONGLONG)(startingSector*(ULONGLONG)2048);
        info.SectorCount        = sectorsPerRead;
        info.TrackMode          = CDDA;
 
        // read data (samples) from CD
        fSuccess = DeviceIoControl(pextr->hCDROM,         
                                  IOCTL_CDROM_RAW_READ,   
                                  &info,                 
                                  sizeof(RAW_READ_INFO), 
                                  smpl,                 
                                  sectorsPerRead * RAW_SECTOR_SIZE,           
                                  &bytesRead,           
                                  0);                   
               
        if(bytesRead != sectorsPerRead * RAW_SECTOR_SIZE) __leave;
        BytesToWrite = bytesRead;
               
        // write data (samples) to hard disk
        fSuccess = WriteFile(hWav, smpl, BytesToWrite, &BytesWritten, 0);
               
        startingSector += sectorsPerRead;

    }
} while((sectorsPerRead >>= 1) != 0);

---

Quote:

---

However, the value of sectorsPerRead CHANGES during the execution of a loop,
even though I do not reset it's value.

---

Which loop? The inner while loop or the outer do-while? If you consider the outer loop, then it is evident that the change to sectorsPerRead occurs in the while condition of the do-while loop.

Is this correct (right shift and assignment operator)?

Code:

---

while((sectorsPerRead >>= 1) != 0);

---

Quote:

---

Which loop? The inner while loop or the outer do-while?

---

It changes during the execution of the inner loop.

Quote:

---

while((sectorsPerRead >>= 1) != 0);

---

Actually, this was to be my next question, as I don't understand what this line is actually doing in the loop.

Quote:

---

Originally Posted by mmscg

However, the value of sectorsPerRead CHANGES during the execution of a loop,
even though I do not reset it's value.

This should be impossible, should it not?

---

It is highly possible if your parameters to DeviceIoControl are not correct.

From MSDN:

Quote:

---

BOOL DeviceIoControl(
HANDLE hDevice,
DWORD dwIoControlCode,
LPVOID lpInBuffer,
DWORD nInBufferSize,
LPVOID lpOutBuffer,
DWORD nOutBufferSize,
LPDWORD lpBytesReturned,
LPOVERLAPPED lpOverlapped
);

---

Please go through all of your parameters that you're passing to DeviceIoControl. So far, I see discrepancies in what you've declared, and what DeviceIoControl expects. You have ULONG, but DeviceIoControl says DWORD. You have other discrepancies in the types you've sent, and the types DeviceIoControl expects.

Since DeviceIoControl is going to write data on return to your buffers that you've passed as parameters, they had better be big enough for the information. If not, DeviceIoControl will inadvertantly cause a memory overwrite, and more than likely, this is what happened. That memory that was overwritten was probably your "sectorsPerRead" variable.

Did the variable change when DeviceIoControl was called? In other words, was sectorsPerRead OK before the call to DeviceIoControl, and then changed after the call to DeviceIoControl? If this is what happened, then it looks like what I've stated happened did happen.

Regards,

Paul McKenzie

Yes, what Paul McKenzie says is highly probable.

Quote:

---

Quote:
while((sectorsPerRead >>= 1) != 0);
Actually, this was to be my next question, as I don't understand what this line is actually doing in the loop.

---

This is the line that is changing the value of sectorsPerRead in the outer loop. Essentially, it shifts the bits of sectorsPerRead by one towards the right. This is a fast and efficient way of dividing by 2. Its the same as saying :

Code:

---

sectorsPerRead /= 2;

---

As a matter of fact, what is this in your code?

Code:

---

smpl

---

You pass this as an output buffer. Nowhere do you state where you've initialized this, or what this is supposed to be. This is probably where things start to break down.

Regards,

Paul McKenzie

I really appreciate the help!!

smpl is a pointer to an output buffer


This is very hard to debug using the debugger because the inner loop gets executed approx 510 times,
and I have to respond to 510 breakpoints :sick: (unless I am still not using the debugger properly).

Is there a way to print to a text file so I can review the variables at each line in the loop.
(I could just print the last 10 executions of the loop as it
seems it is not until this point that sectorsPerRead starts changing)

Quote:

---

Originally Posted by mmscg

I really appreciate the help!!

smpl is a pointer to an output buffer

---

We all know it's a pointer to an output buffer.

Please, can you be a little more specific? Where is it initialized? What is it declared as? Where does it point to initially? Where is this "buffer" that it points to?

It isn't a matter of the debugger, it is a matter of understanding what you've programmed. When an API function asks for a pointer to a buffer, for the most part, the API function is assuming you are pointing to somewhere that is valid, and is big enough to hold the information when the function is executed.

If you just declare an LPVOID or whatever, and do not point it anywhere, of course you'll have memory overwrites leading to weird behaviour and crashes.

Regards,

Paul McKenzie

Quote:

---

Originally Posted by mmscg

This is very hard to debug using the debugger because the inner loop gets executed approx 510 times,

---

Why not change your program so that you create your own breakpoint when the code doesn't act correctly?

Code:

---

// read data (samples) from CD
        ULONG testValue = sectorsPerRead;

        fSuccess = DeviceIoControl(pextr->hCDROM,         
                                  IOCTL_CDROM_RAW_READ,   
                                  &info,                 
                                  sizeof(RAW_READ_INFO), 
                                  smpl,                 
                                  sectorsPerRead * RAW_SECTOR_SIZE,           
                                  &bytesRead,           
                                  0);                   

        if ( testValue != sectorsPerRead )
        {
              // what happened?
              testValue = sectorsPerRead  // put a breakpoint here
        }
}

---

Very simple. Just create code where you know it will only be reached if there is a problem, and put a breakpoint in that code.

But really, the problem is more than likely your output buffer. Please, next time when code is posted, if you are going to use variables, post where they came from, how they are initialized, etc. Variables that just appear out of thin air in code snippets are notorious for being the cause of problems (for example, the "smpl" variable).

Regards,

Paul McKenzie

Code:

---

PUCHAR    smpl;
smpl = pextr ->pmem _ sizeof(CDROM_TOC) + RAW_SECTOR_SIZE;

---

I don't think sectorsPerRead is in fact a problem
(other than the fact that it gets changed somewher and I don't know why or how).

As I said this code works:

Code:

---

        // read data (samples) from CD
        fSuccess = DeviceIoControl(pextr->hCDROM,         
                                  IOCTL_CDROM_RAW_READ,   
                                  &info,                 
                                  sizeof(RAW_READ_INFO), 
                                  smpl,                 
                                  sectorsPerRead * RAW_SECTOR_SIZE,           
                                  &bytesRead,           
                                  0);                   
               
        if(bytesRead != sectorsPerRead * RAW_SECTOR_SIZE) __leave;

---

This does not:
(so it makes sense that bytesRead does not equal bytesPerRead and the inner loop is exited
before it finishes ripping CD data)

Code:

---

        bytesPerRead = sectorsPerRead * RAW_SECTOR_SIZE;

        // read data (samples) from CD
        fSuccess = DeviceIoControl(pextr->hCDROM,         
                                  IOCTL_CDROM_RAW_READ,   
                                  &info,                 
                                  sizeof(RAW_READ_INFO), 
                                  smpl,                 
                                  bytesPerRead,           
                                  &bytesRead,           
                                  0);                   
               
        if(bytesRead != bytesPerRead) __leave;

---

I can understand everyones frustration with me,
but believe me I am trying to figure this out on my own and not waste forum space.

Quote:

---

Why not change your program so that you create your own breakpoint when the code doesn't act correctly?

---

Awesome suggestion!! :D

Quote:

---

Originally Posted by mmscg

Code:

---

PUCHAR    smpl;
smpl = pextr ->pmem _ sizeof(CDROM_TOC) + RAW_SECTOR_SIZE;

---

---

According to the documentation for DeviceIoControl, the buffer that you pass to it must be big enough to hold the information that will be written. What you posted there doesn't mean anything to anyone looking at your code, i.e. it doesn't convey how big the buffer is. The bottom line is this.

Can you guarantee that the buffer that is being pointed to is

1) valid (no one can tell by looking at your code)

AND

2) large enough to hold the information returned by DeviceIoControl (again, no one can tell by looking at your code)

If either 1) or 2) are not true, then your code is faulty.

Why not do something simple -- make smpl point to a buffer that is 10,000 bytes and forget about all of that other stuff temporarily?

Code:

---

UCHAR    smpl[10000];
//smpl = pextr ->pmem _ sizeof(CDROM_TOC) + RAW_SECTOR_SIZE;

---

Now, does the loop perform more times? Does the memory overwrite occur? If it doesn't occur, then the problem is the output buffer you were providing to DeviceIoControl -- it was either too small, or it was pointing to somewhere invalid (or both).

Quote:

---

I don't think sectorsPerRead is in fact a problem
(other than the fact that it gets changed somewher and I don't know why or how).

---

I think I've been trying to tell you the why and how this can happen. Your call to DeviceIoControl is more than likely faulty because of a memory overwrite.

Quote:

---

As I said this code works:

---

We know what works and what doesn't. We are trying to convey to you why it doesn't work and what is the probable cause. Anyone who has experience in the language will tell you that the call to DeviceIoControl, given what all of those parameters mean and what DeviceIoControl does, is what you should be really inspecting.

Regards,

Paul McKenzie

Quote:

---

Can you guarantee that the buffer that is being pointed to is

1) valid (no one can tell by looking at your code)

AND

2) large enough to hold the information returned by DeviceIoControl (again, no one can tell by looking at your code)

If either 1) or 2) are not true, then your code is faulty.

---

I am 99% sure that the buffer is both valid and large enough to hold return info from DeviceIoControl call.

Quote:

---

Why not do something simple -- make smpl point to a buffer that is 10,000 bytes and forget about all of that other stuff temporarily?

---

I will perform this test tonight.

Quote:

---

Now, does the loop perform more times? Does the memory overwrite occur? If it doesn't ...

---

I'm not sure what you mean by the memory overwrite?

Quote:

---

Originally Posted by mmscg

I am 99% sure that the buffer is both valid and large enough to hold return info from DeviceIoControl call

---

99% percent sure is not a guarantee. You must be 100% sure that the buffer is large enough to hold the information, otherwise the code has to be rewritten. Also, it's hard for us helping you to say "OK" to what you're saying. Unless we have the code ourselves, or could decipher what all of that was that you posted, the buffer being too small is still on the table as the reason for the problem.

Quote:

---

I'm not sure what you mean by the memory overwrite?

---

Code:

---

#include <string.h>
void MyOwnDeviceIoControl(char *pOut, int* pOutLength)
{
    int OutputLength = 100;
    memset(pOut, 0, OutputLength);  // memory overwrite
    *pOutLength = 100;
}

int main()
{
    int xyz = 149;
    char whatever[10];
    int length;
    MyOwnDeviceIoControl(whatever, &length);
    // xyz has changed
}

---

I ran this on Visual C++ 6.0. The value of xyz is 149. However, after the call to MyOwnDeviceIoControl is made, xyz has mysteriously changed to 0.

This is exactly the scenario that you're describing. The "whatever" buffer was too small, since the MyOwnDeviceIoControl function assumes that you are pointing to a buffer that can hold at least 100 bytes. When the function does the memset() it writes 100 bytes starting at the "whatever", and like a bull in a china shop, overwrites anything in its way, including the xyz variable. This is a classic memory overwrite.

Of course, the code may crash and burn on the memset(), but this is an illustration of why I believe your buffer is not big enough, even though you say it is.

Regards,

Paul McKenzie

I think you may need to rewrite the code from scratch. There are quite a few inconsistencies in the way you used DeviceIoControl() and the way msdn declares it. The types used also do not match as pointed out by Paul.

1. if info is the input buffer, then the next argument should be sizeof(info) instead of sizeof(RAW_READ_INFO). If RAW_READ_INFO has been defined as a macro then sizeof(RAW_READ_INFO) will be 4 bytes.

2. Likewise, I'm not sure about your output buffer. Are you certain that sectorsPerRead * RAW_SECTOR_SIZE is the size of the output buffer? Or for that matter is bytesPerRead?

3. Finally, if you think about it, the program goes on reading data from the CD and filling the output buffer with the data. Ultimately, when it comes to the last block of data, the output buffer may not be completely filled, in which case bytesRead will not be equal to bytesPerRead. In other words, this is not a proper way to check whether data has been read or not. To see the proper way, refer this link http://msdn2.microsoft.com/en-us/library/aa363216.aspx

Quote:

---

...and like a bull in a china shop, overwrites anything in its way...

---

Very Effectively put. :D

Quote:

---

I can understand everyones frustration with me,
but believe me I am trying to figure this out on my own...

---

We believe you. Your frustration has become our frustration. :) Unlike some of the other lazy programmers out there, at least you are trying to figure it out. That's commendable. :thumb:

Quote:

---

Why not change your program so that you create your own breakpoint when the code doesn't act correctly?

if ( testValue != sectorsPerRead )
{
// what happened?
testValue = sectorsPerRead // put a breakpoint here
}

---

If statement never executed.


So I printed out variables at each loop iteration (code as posted loops 508 times).

... sectorsPerRead automagically changes from 27 (as I had assigned it) to 6 and then 1

Code:

---

  i    bytesRead    bytesPerRead    sectorsPerRead *    sectorsPerRead    fSuccess
                                    RAW_SECTOR_SIZE
----    ---------    ------------    ----------------    --------------    --------
  .
  .
  .
 501        63504          63504              63504                27          0
 502        63504          63504              63504                27          0
 503        63504          63504              63504                27          0
 504        63504          63504              63504                27          0
 505        14112          63504              14112                6          0
 506        2352          63504                2352                1          0
 507        2352          63504                2352                1          0
 508        2352          63504                  0                0          0

---

2352 X 27 = 63504
2352 X 6 = 14112
2352 X 1 = 2352

Quote:

---

Why not do something simple -- make smpl point to a buffer that is 10,000 bytes and forget about all of that other stuff temporarily?

---

I did this and now code loops 4,549 times .

I changed the buffer size to 10,000 bytes, but changed nothing else
(sectorsPerRead is left equal to 27).

Printing out the variables at each loop iteration now yields:

(and sectorsPerRead now automagically becomes 3)

Code:

---

  i    bytesRead    bytesPerRead    sectorsPerRead *    sectorsPerRead    fSuccess
                                      RAW_SECTOR_SIZE
----    ---------    ------------    ----------------    --------------    --------
  .
  .
  .
4542        7056          63504                7056                3          0
4543        7056          63504                7056                3          0
4544        7056          63504                7056                3          0
4545        7056          63504                7056                3          0
4546        7056          63504                7056                3          0
4547        7056          63504                7056                3          0
4548        7056          63504                7056                3          0
4549        2352          63504                  0                0          0

---

2352 X 3 = 7056


angelorohit:

Quote:

---

I think you may need to rewrite the code from scratch.

---

I think you are right.

Quote:

---

2. Likewise, I'm not sure about your output buffer. Are you certain that sectorsPerRead * RAW_SECTOR_SIZE is the size of the output buffer? Or for that matter is bytesPerRead?

---

Yes, see above data printouts.

Quote:

---

3. Finally, ... ...To see the proper way, refer this link http://msdn2.microsoft.com/en-us/library/aa363216.aspx

---

The link is invalid.



Again, thanks all for the help!!

Since you still refuse to show us the definitions of your buffer and your variable sectors_per_read I think you still haven't understood what everybody here thinks is happening in your program.
I give you some example

Code:

---

#include <iostream>
#include <string>
#include <iomanip>

using namespace std;

int main() {

    int sectors_per_read = 100;
    char buffer[15];

    cout << "before out of bounds assignement" << endl;
    cout << left << setw(20) << "sectors_per_read " << sectors_per_read << endl;


    // overflow buffer
    // something like this might happen with your DeviceIoControl() call if you
    // specify a wrong buffer size.
    for ( int i= 0; i < 32; ++i )
        buffer[i] = 1;

    cout << "after out of bounds assignement" << endl;
    cout << left << setw(20) << "sectors_per_read " << sectors_per_read << endl;
}

---

my output

Code:

---

before out of bounds assignement
sectors_per_read    100
after out of bounds assignement
sectors_per_read    16843009

---

Your compiler might allocate different amount of memory for that variables, so this example might crash on your system or it might appear to work correctly.
Hope you understand now how it's possible that variables change their value automagically.
Kurt

You'll probably find that the best search tool for programming is Google. Google seem to have indexed all of Microsoft's help pages, vast numbers of newsgroups, and all of sites like CodeGuru.

And then, if Google doesn't turn up a useful answer, I'd search other sources.

Most modern debuggers let you set conditions on the breakpoint so you don't have to respond to the first 'n' iterations throught loops, etc. If you have a loop counter, or some other variable whose state indicates when you really want to break, have the condition test it for the trigger value.

I've figured it out!!

Well actually angelorohit did.

In trying to understand how the exact amount of audio sectors gets copied to hard drive, when audio will not always be a multiple of 27,
it hit me that that was the purpose of the outer loop - to adjust sectorsToRead to compensate for this in the last few loop iterations.

angelorohit tried to tell me that way back in post #15

Quote:

---

This is the line that is changing the value of sectorsPerRead in the outer loop. Essentially, it shifts the bits of sectorsPerRead by one towards the right. This is a fast and efficient way of dividing by 2. Its the same as saying :
Code:
sectorsPerRead /= 2;

---

When I try to read more sectors than are actually left in the audio being copied,
the inner loop is exited, and the sectorsPerRead is adjusted to [b]half the previous amount (27/2=13),
it checks again, and again 13 is more than the sectors left so
inner loop is exited again and sectorsPerRead is again adjusted to half the previous amount (13/2=6),
this time it is able to read & copy 6 sectors,
This is repeated until all the remaining sectors have been read.

I won't be able to verify this 'till later, but I'm certain this is what is happening.

Thanks for all the help!!

Ah, finally! Glad to hear I could be of some assistance. You see? My version of Murphy's law states that the offending statement will be the one that you don't understand the most. That is why I spoke about the outer loop and the shift operator - to make sure that you know what's happening.