Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenCV 3.1 x64 -- Incorrect code is generated when calling cvGetSize from a x64 C library. #6221

Closed
rovarma opened this issue Mar 8, 2016 · 4 comments

Comments

@rovarma
Copy link

rovarma commented Mar 8, 2016

Please state the information for your system

  • OpenCV version: 3.1
  • Host OS: Windows 8
  • Compiler & CMake: MSVC 2015 x64 & CMake 3.5

In which part of the OpenCV library you got the issue?

  • core

Description

When using OpenCV 3.1 from a x64 C library, incorrect code is generated when calling cvGetSize. I have only seen this when compiling for x64 (I only have x64 projects), but I don't know whether the bug also appears when building for x86.

The reason for this bug is the fact that the CvSize struct is defined as follows:

typedef struct CvSize
{
    int width;
    int height;

#ifdef __cplusplus
    CvSize(int w = 0, int h = 0): width(w), height(h) {}
    template<typename _Tp>
    CvSize(const cv::Size_<_Tp>& sz): width(cv::saturate_cast<int>(sz.width)), height(cv::saturate_cast<int>(sz.height)) {}
    template<typename _Tp>
    operator cv::Size_<_Tp>() const { return cv::Size_<_Tp>(cv::saturate_cast<_Tp>(width), cv::saturate_cast<_Tp>(height)); }
#endif
} CvSize;

Since OpenCV itself is compiled as C++ (not as C), this means that CvSize has constructors (#ifdef __cplusplus == true), which makes it a user-defined type. This in turn leads the MSVC compiler to assume that functions returning this struct by value cannot return it in a register, which causes the following code to be generated for cvGetSize (note: this is just the function prologue):

// Returns the size of CvMat or IplImage
CV_IMPL CvSize
cvGetSize( const CvArr* arr )
{
00007FF763B006F0  mov         qword ptr [rsp+10h],rdx  
00007FF763B006F5  mov         qword ptr [rsp+8],rcx  
00007FF763B006FA  push        rdi  
00007FF763B006FB  sub         rsp,70h  

Note that up until this point, everything is still correct. cvGetSize is a function taking two arguments: the address where the return value of cvGetSize should be stored is passed in rcx, the arr argument is passed in rdx.

However, when you try to call this function from a file compiled as C (not C++), things break down. This is because the constructors in CvSize dissapear (#ifdef __cplusplus == false), which now leads the compiler to assume that the return value for cvGetSize will be returned in a register (rax) since sizeof(CvSize) == 8.

So, when calling the function, only one parameter is pushed onto the stack (the arr argument), which mismatches with what the generated code for CvSize internally expects. This leads to the following (incorrect) calling code:

    CvSize frame_size = cvGetSize(frame);
00007FF7637E2BF3  mov         rcx,qword ptr [frame]  
00007FF7637E2BF7  call        cvGetSize (07FF763353B8Ah)  
00007FF7637E2BFC  mov         qword ptr [frame_size],rax

Note a few things here:

  • The pointer to frame is stored in rcx, which is where cvGetSize expects to find the address where the return value should be stored.
  • The calling code expects the return value from cvGetSize to be stored in rax, which will not be the case

All of this means that cvGetSize will read its argument from the wrong address and either throw an error or crash.

However, when you instead call this function from a file compiled as C++ (not C), correct code is generated:

    CvSize frame_size = cvGetSize(frame);
00007FF722682C2B  mov         rdx,qword ptr [frame]  
00007FF722682C2F  lea         rcx,[frame_size]  
00007FF722682C33  call        cvGetSize (07FF7221F3B85h)

As you can clearly see, 2 arguments are passed to the function; the address where the return value should be stored in rcx and the arr argument to the function in rdx. This matches with the internally generated code for cvGetSize, so everything works as expected.

I think the current code probably works fine on other compilers (I haven't tried), but that's more of an accident than an intended feature. I believe this is firmly in undefined behaviour territory, so really anything could happen; the code is incorrect.

To fix this, I think the constructors should be removed from CvSize. There are probably also other structs that may suffer from similar issues (at a glance, CvSize2D32f, CvBox2D all have a similar problem).

Some links that might be useful:

https://msdn.microsoft.com/en-us/library/1e02627y.aspx
http://stackoverflow.com/questions/22901697/error-in-c-code-linkage-warning-c4190-type-has-c-linkage-specified-but-retu

Code example to reproduce the issue / Steps to reproduce the issue

  • Create (or reuse) a x64 Visual Studio project which is setup to include/link against OpenCV 3 x64

  • Create a main.c file

  • Put the following code in there:

    #include "opencv2/core/core_c.h"
    
    int main(int _argc, char** _argv)
    {
        IplImage* frame = cvCreateImage(cvSize(640, 480), IPL_DEPTH_32F, 3);
        CvSize frame_size = cvGetSize(frame); // This line will result in an exception being thrown
    }
    
  • Right-click on main.c in Visual Studio and go to Properties -> C/C++ -> Advanced

  • Set the Compile As field to Compile as C code (/TC)

  • Put a breakpoint on the line calling cvGetSize and run the program

  • Make note of the pointer value of frame

  • Step into cvGetSize

  • Notice that the argument (arr) is not the correct pointer value (ie. it is not pointing to frame).

  • Press F5 to continue running the code and note the error (CV_Error( CV_StsBadArg, "Array should be CvMat or IplImage" );)

  • Now set the Compile As field of main.c to Compile as C++ code (/TP)

  • Run again

  • Notice that the argument to cvGetSize is now correct and no error is thrown

@rovarma
Copy link
Author

rovarma commented Mar 9, 2016

Just realized I forgot to include an important piece of information: I've only seen this happening when building for x64 (which doesn't mean that it works for x86, just that I've only tested x64). I've updated the original issue with this information.

@rovarma rovarma changed the title OpenCV 3.1 -- Incorrect code is generated when calling cvGetSize from a C library. OpenCV 3.1 x64 -- Incorrect code is generated when calling cvGetSize from a x64 C library. Mar 9, 2016
rovarma added a commit to rovarma/psmoveapi that referenced this issue Mar 9, 2016
…forms now work with the same OpenCV version (3.1.0)

- Updated MSVC build script to clone the 3.1.0 branch. Also updated the cmake commandline (3.1.0 needs opencv_ml to be built as well)
- Updated MSVC readme to use 3.1.0
- Updated osx build script to also clone 3.1.0 branch instead of whatever is currently at master
- Due to a bug in OpenCV 3.1.0 it is not possible to call cvGetSize from C source files under MSVC (it will crash due to incorrect code generation). As a workaround for this (until the bug in OpenCV is fixed), the psmove_tracker.c file (which is the only code calling cvGetSize) is now force-compiled as C++. See opencv/opencv#6221 for more info.
- In order to be able to build psmove_tracker.c as C++, some code needed to be updated (extern "C" added where relevant, tracker_default_settings can no longer be initialized using C-style designated initializer)
@alalek
Copy link
Member

alalek commented Jul 8, 2016

C API is not supported anymore and should not be used. Moreover some "C" calls can raise C++ exceptions.
There is no plan to revive direct C API in OpenCV (you should wait for some FFI C bindings). Use OpenCV from C++ programs only.

#4705

@rovarma
Copy link
Author

rovarma commented Jul 8, 2016

Lol. That is "a" response, I suppose.

@ourway
Copy link

ourway commented Jun 11, 2019

C API is not supported anymore and should not be used. Moreover some "C" calls can raise C++ exceptions.
There is no plan to revive direct C API in OpenCV (you should wait for some FFI C bindings). Use OpenCV from C++ programs only.

#4705

This is terrible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants