0

I'm trying to teach myself C++ (actually I should say re-learn, but I first learned it when I didn't know a thing about coding and year ago so it doesn't count) and I'm doing my first project after finishing the online tutorial. I figured since I had a good C# and VB.Net background I might as well try something a bit bigger, but not too big. Before I start, I'm using Code::Blocks as my IDE and the default compiler in that IDE (I believe it's MinGW). So here is my thing : I have a ChromaTest project (this is using the Razer Chroma SDK for those wondering about the name) which is a console app, and a ChromaTestDLL project, which is (you guessed it) a DLL (I decided to do a DLL to learn how to do so at the same time and because I might use some the code in a GUI project later on). Problem is I'm getting a Segmentation Fault error when trying to insert into a map. Here is the relevant code :

In the ChromaTestDLL Project

MyChroma.h (Header for the MyChroma class)

#ifndef MYCHROMA_H
#define MYCHROMA_H

#include <map>
#include <windef.h>
#include "RzChromaSDKTypes.h"
#include <string>
#include "Template.h"

#ifdef BUILD_DLL
    #define DLL_EXPORT __declspec(dllexport)
#else
    #define DLL_EXPORT __declspec(dllimport)
#endif

using namespace std;

#ifdef __cplusplus
extern "C"
{
#endif

class DLL_EXPORT MyChroma
{
    public:
        MyChroma();

        bool Init();

        std::map<char, COLORREF> GetColorMapping();

        void SetColorMapping(char key, COLORREF color);

        void AssignToKeyBoard();

        void SetColorFromString(string s, COLORREF color);

        ~MyChroma();

    protected:
        std::map<char, COLORREF>* _ColorMapping;

        ChromaSDK::Keyboard::RZKEY KeyFromChar(char keyChar);

        My_Chroma_Implementation* Chroma;

    private:
};

#ifdef __cplusplus
}
#endif

#endif // MYCHROMA_H

MyChroma.cpp (Relevant implementation for MyChroma class)

#include "MyChroma.h"
#include "Template.h"
#include <iostream>

MyChroma::MyChroma()
{
     _ColorMapping = new std::map<char, COLORREF>();
}

std::map<char, COLORREF> MyChroma::GetColorMapping() { return *_ColorMapping; }

void MyChroma::SetColorMapping(char key, COLORREF color){

    if (_ColorMapping->count(key) == 0)
        _ColorMapping->insert(std::make_pair(key, color)); //This where the error happens
    else
        (*_ColorMapping)[key] = color;
}

MyChroma::~MyChroma() {
    delete Chroma;
    delete _ColorMapping;
}

//Other implementations omitted

In the ChromaTest project

MyChroma.h (Header to import MyChroma class, slightly different from the one in ChromaTestDll, basically it only contains public members)

#ifndef MYCHROMA_H
#define MYCHROMA_H

#include <map>
#include <windef.h>
#include <string>

#ifdef BUILD_DLL
    #define DLL_EXPORT __declspec(dllexport)
#else
    #define DLL_EXPORT __declspec(dllimport)
#endif

using namespace std;

#ifdef __cplusplus
extern "C"
{
#endif

class DLL_EXPORT MyChroma
{
    public:
        MyChroma();

        bool Init();

        std::map<char, COLORREF> GetColorMapping();

        void SetColorMapping(char key, COLORREF color);

        void AssignToKeyBoard();

        void SetColorFromString(string s, COLORREF color);

        ~MyChroma();

};

#ifdef __cplusplus
}
#endif

#endif // MYCHROMA_H

Main.cpp (main app code)

#include <iostream>
#include "MyChroma.h"

#include <wingdi.h>

using namespace std;

int main()
{
    MyChroma test = MyChroma();
    bool result = test.Init();

    cout << (result ? "Initialized\n" : "Failed to initialize Razer Chroma");

    cout << "Setting color";

    if (result){
        test.SetColorMapping('a', RGB(255,0, 0)); //This call goes in the DLL where I said it failed earlier.
        test.SetColorMapping('a', RGB(0,0,255));
    }

    return 0;
}

Sorry for the atrociously long code (please tell me if there are things I could have removed here). Can anybody spot any mistake in there I would not be surprised this would be linked to pointers, this is probably the concept which took me the most time to understand. At first I didn't put the map in a pointer and on the heap, but changing another variable to that earlier seemed to have fixed another problem so I figured I'd give it a try. Sadly I had pretty much the same errors when not putting the map on the heap too.

On a side note, can anybody explain to me what different between the heap and the stack, why would I want to go through the (risky) hassle of storing variables on the heap (with pointers and deletion and all) instead of on the stack, and when should I use the heap or when should I not.

Noémie Lord
  • 751
  • 4
  • 22
  • 4
    Good news! Today, on stackoverflow.com, we are running a promotion called "Answer Your Own Question Day". We have secretly placed on your computer a highly advanced tool called a "debugger". Using this amazing tool you can execute your program one line at a time, examine all values of all variables, and "debug" your code, thus answering your own question. Don't miss this exciting opportunity to Answer Your Own Question, today only, on stackoverflow.com! – Sam Varshavchik Sep 11 '16 at 01:23
  • 2
    You broke the Rule of Three, and now it has returned to break you. – Kerrek SB Sep 11 '16 at 01:24
  • Why on earth would you declare your `std::map` as a pointer?? Not event a unique... – Guillaume Racicot Sep 11 '16 at 01:32
  • @SamVarshavchik I have tried debugging. A lot. I have checked that the map pointer is not null, the values given are not null either. However I have almost no experience with the environment, and I don't really know C++'s errors. Hence why I asked SO for help. – Noémie Lord Sep 11 '16 at 01:32
  • @GuillaumeRacicot errrr, maybe cause this is my first ever project in C++ and I have no idea when and when not to use pointers and the heap? – Noémie Lord Sep 11 '16 at 01:33
  • Ok, after deciphering what you did, your major malfunction is very obvious... – Sam Varshavchik Sep 11 '16 at 01:34
  • @SamVarshavchik hit me with your best shot! ;) – Noémie Lord Sep 11 '16 at 01:37

1 Answers1

2

Based on the information in your question:

  1. The compiled code in your DLL appears to declare a MyChroma class containing a bunch of internal class members, in its header file.

  2. Then your main application uses a completely different header file, that defines a class called MyChroma, stripped of its class members.

  3. Then, your main application instantiates the MyChroma class, based on what it sees in its header files.

That's not going to work. Since your main application knows nothing about those class members, the actual class that it instantiates is too small.

And it instantiates a class on the stack.

And then the constructor comes from the DLL, which thinks the class contains all those other class members.

And the constructor in the DLL attempts to initialize them.

On the stack.

Hello stack corruption.

The answer here is simply "don't do what you did". This is undefined behavior. Everything that you compile that references a particular class must see an identical declaration (and inline method definitions) of the class.

Full stop.

No exceptions.

Well, with sufficient experience, it's possible to do something like this safely, when targetting a specific C++ implementation, but this is not the case here.

Until then, there are ways to hide the internal implementation details of library-provided classes, but this is not how you do it. The safe way to do it is with the PIMPL design pattern.

A few other thing you should not do, as well. This doesn't relate directly to the problem at hand, but this will avoid several other common pitfalls that can, without advance warning, pull the rug from under your feet:

  1. Do not use use namespace std;. Especially in header files. Completely forget that something like that exists in the C++ language.

  2. All your classes should also follow the Rule Of Three.

Community
  • 1
  • 1
Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
  • Thanks for pointing out the flaws in my design / implementation. I will look into what you have pointed linked and remake the project from the almost ground up. – Noémie Lord Sep 11 '16 at 03:07