Illegal read/write error when making legacy code x64 compliant

Question

I have the following MyType::Is_Inst () function which is throwing an invalid memory access error on return in 64-bit mode but not in 32-bit:

MyType MyType::Is_Inst () {
    unsigned char Bar=0;    
    MyType Foo={0};    
    return Foo;
    }

Looking at the disassembly + step-through, the program crashes at the line

mov  dword ptr [rax],ecx

...when the program is basically trying to dereference the original value of %rdx (from when the function was first called), which is now in %rax. However, %rdx is just junk left over from the previous function call.

The last time I had an issue like this, it was because I was missing some compilation flags or the like. Are there any settings I should be aware of for x64 unmanaged c++ projects? Are there any other reasons I might see this behavior?

I can post more of the disassembly if you need it.

The class definition for MyType looks like this:

class __declspec(dllexport) MyType {
public:
    union {
        struct {
            unsigned int    Id     : 23;
            unsigned int    Flag   : 1;  
            unsigned int    Type   : 4;  
            unsigned int    Unused : 4;  /* 32 bits total */
            };

        unsigned int    All_Bits;          /* Full 32 bits of MyType */
        };

     /* There are some function definitions here, but no other 
        variables, aside from some statically defined ones. */
     };

UPDATE: This is the more succinct, optimized version of Is_Inst()'s disassembly, which shows the problem. I've removed the old version that was here before for brevity.

// MyType MyType::Is_Inst () {
// uchar Bar=0;
// MyType Foo={0};
   mov         dword ptr [rdx],0 /* %rdx is 0x17, from a prev fn call. */ 

// return Foo;
   mov         rax,rdx  
// }
   ret

The code leading up to Is_Inst() getting called:

...
for (Counter=0; Counter<N_Items; Counter++) {
    ...
    myOther32BitType = arrayOfMyOtherTypes [Counter]; /* Debugger shows this is ok. */ 

    if (myOther32BitType.8BitField==UNEQUAL_ENUM_VALUE) {
        /* Some stuff that doesn't happen. */
        }

    /* myOther32BitType.8BitField==0x17, so %rdx gets set to 0x17. */
    else if (strchr((char*)Uchar_Table_Of_Enum_Values, myOther32BitType.8BitField)) continue;

    /* %rdx gets set to 0x17 again. */
    else if (strchr((char*)Other_Uchar_Table_Of_Enum_Values,  myOther32BitType.8BitField)) continue;

    else if ( myOther32BitType.8BitField==EQUAL_ENUM_VALUE) {
        if (myType.Is_Inst ().All_Bits) { /* Is_Inst() called here. */
            return false;
            }
        ...
        }
    ...
    }

Sounds like memory corruption elsewhere in the code. Crank up your compiler's warning level and keep an eye out for data truncation warnings. Also keep an eye out for code that assumes it knows the size of fundamental types. — ildjarn, Jun 06 '11 at 21:11
Yeah, that's what I'm currently doing. Unfortunately, this code has so many offenses it's painful. :P — Amanduh, Jun 06 '11 at 21:14
Having gone through the disassembly by hand, though, the code really is just taking the previous value of %rdx and storing it to %rax, then dereferencing it. What I don't understand is why the compiler is ever doing either of those steps at all. Both of those steps are done inside of the Is_Inst fn... — Amanduh, Jun 06 '11 at 21:27
What does the copy constructor for MyType look like? Since you are returning Foo off the stack the copy constructor needs to get invoked to copy it to whatever it is getting assigned to. Maybe the problem is there? — ribram, Jun 06 '11 at 21:30
@Alan Stokes, I've updated the post to include the definition for MyType. @ribram: I assume you mean the overloaded '=' operator? As far as I can tell, there isn't one. — Amanduh, Jun 06 '11 at 21:34
I might be off here, but I have this sneaking suspicion that in a 32-bit compile, the size of the All_bits member of the union and of the bit field will both be 32 bits whereas in a 64 bit compile, chances are that All_bits is 64 bits wide. You might want to use something it uint32_t in both cases... — Timo Geusch, Jun 06 '11 at 21:49
@Tim Geusch, I'll give it a try and let you know how it goes... This class is the basis for everything else we are doing, so it might take awhile before I can say for sure. — Amanduh, Jun 06 '11 at 21:54
If you were on Linux I'd suggest using valgrind. It may be possible to use it on Windows too, or some other equivalent tool to help you find all illegal memory accesses, not just ones that happen to crash right now. — John Zwinck, Jun 07 '11 at 01:23
It's possible it really is a code generation bug. I'd be interested to see the complete disassembly of the function. — Alan Stokes, Jun 07 '11 at 07:16
Are you absolutely sure rdx is not used for either this or (more likely in my opinion) location of return value? Could you show the line which calls Is_Inst and is causing problems along with possible variable declarations? — Tomek, Jun 07 '11 at 10:21
@Tim Geusch, I tried you suggestion, but it doesn't seem to change anything. @Alan Stokes, @Tomek, I've added the fn's disassembly. I might be reading it incorrectly, so feel free to say as much. Thanks everyone for your help. Even though this isn't solved yet, I really appreciate getting so much support from everyone. — Amanduh, Jun 07 '11 at 14:25
Are `Is_Inst` and the loop that calls it both built using the same compiler and using the same options? Also `mov [Foo], 0` looks a bit weird... I would expect something relative to `rsp`/`rbp` for this since `Foo` is on the stack... — asveikau, Jun 07 '11 at 17:03
@asveikau, Yes, they are both in the same VS project. I can't explain the `mov [Foo], 0`, except that this is the disassembly VS gives me. The debugger shows that the address of foo is inside the giant 0xcccccccc block pointed to by %rsp. Specifically, it's at something like `rsp+0x34`. — Amanduh, Jun 07 '11 at 17:11
I guess where I was going with this is it almost looks like the callee is expecting the caller to specify an out parameter that will receive the return value. (In MS land on amd64, the first two parameters are passed in `rcx` and `rdx`. I assume `rcx` at function start is the `this` pointer here.) It seems like the callee expects this but the caller is not doing it. Could there be some kind of mismatch between the header and the implementation? Or between compilation units? — asveikau, Jun 07 '11 at 17:15
@asveikau, Yeah, `rcx` is the `this` pointer. I wondered something of the sort myself, as well, which is why I thought maybe I'm missing some x64 compilation option. The header matches the implementation, from what I can see. Now that someone else shares the same suspicions as me, I think I'll research it a bit more in-depth... — Amanduh, Jun 07 '11 at 18:31
I was also wondering if `__declspec(dllexport)` might somehow alter the behavior. A long shot, but I've seen stranger things than that. — asveikau, Jun 07 '11 at 18:58
@Timo Geusch, Also, just for the record, I did check the size of MyType in 64-bit mode, and it comes back as 4 bytes. — Amanduh, Jun 08 '11 at 18:28

score 1 · Accepted Answer · answered Jun 08 '11 at 16:58

1

I've posted a bug report to MS here: https://connect.microsoft.com/VisualStudio/feedback/details/674672/callee-disassembly-expects-address-which-caller-is-not-providing-in-x64-mode

Again, thanks to all for your help!

answered Jun 08 '11 at 16:58

Amanduh

1,233
1
13
23

Simply adding another parameter to the Is_Inst's declaration (e.g., 'nid Is_Inst (int Dummy=0);') in the header file appears to be functioning as a work around. O__o – Amanduh Jun 24 '11 at 21:20

score 0 · Answer 2 · answered Jun 07 '11 at 15:05

0

This seems like the same bug Qt people encountered in VS2010. The report links to Microsoft Connect here. And the bug should be fixed in SP1. There were numerous optimization b ugs fixed in SP1, so I may not have linked the correct one (found three others just sifting through links).

answered Jun 07 '11 at 15:05

rubenvb

74,642
33
187
332

:/ I so hoped you were right, but after installing SP1, the issue still shows up. More likely than not it's something on our side, not on MS's side, but it was worth a shot. Thanks anyway! – Amanduh Jun 07 '11 at 16:16

score -1 · Answer 3 · answered Jun 08 '11 at 19:34

-1

else if (strchr((char*)Other_Uchar_Table_Of_Enum_Values,  myOther32BitType.8BitField)) continue;

This looks horrifically wrong. Instead of resorting to completely unnecessary casting, you should use std::find.

In fact, your whole code looks littered with C-style unsafe code. Where's the memory management objects?

answered Jun 08 '11 at 19:34

Puppy

144,682
38
256
465

This is unmanaged code. Please also note that this isn't my code. It's five years worth of accumulated code that I am now responsible for converting. – Amanduh Jun 08 '11 at 20:47

Illegal read/write error when making legacy code x64 compliant

3 Answers3