open62541pp / open62541pp

C++ wrapper of the open62541 OPC UA library
https://open62541pp.github.io/open62541pp/
Mozilla Public License 2.0
102 stars 36 forks source link

checkIsDataType fails when used in multiple translation units #251

Closed pemartin2 closed 6 months ago

pemartin2 commented 6 months ago

Hi Lukas,

Found the following bug in the current implementation at least in master and v0.12.0.

The problem is that we compare addresses in this method and not values (not sure if it's valid to just compare the typeIdhere @lukasberbuer ?). If executed from different translation units this results in exception.

    template <typename T>
    void checkIsDataType() const {
        if (getDataType() != &opcua::getDataType<T>()) {
            throw BadVariantAccess("Variant does not contain a value convertible to template type");
        }
    }

Example code to reproduce: CMakeLists.txt

cmake_minimum_required (VERSION 3.21)
project ("CMakeProject1" LANGUAGES CXX)
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
find_package(open62541pp CONFIG REQUIRED)

add_executable (CMakeProject1 "main.cpp")
target_link_libraries(CMakeProject1 PRIVATE myLib)

add_library(myLib SHARED
        serverWrapper.hpp
        serverWrapper.cpp
)

target_include_directories(myLib
        PUBLIC
        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
)

target_compile_definitions(myLib PRIVATE WRAPPER_LIBRARY)

target_link_libraries(myLib PUBLIC open62541pp::open62541pp)

main.cpp

#include "serverWrapper.hpp"
int main()
{
    serverWrapper server;
    // everything in same TU -> ok
    server.executeInOneTranslationUnit();

    // different TU -> error
    auto asVariant = server.m_server->getNode({ 1, "Test" }).readValue();
    auto datatype1 = asVariant.getDataType();
    auto dataype2 = &opcua::getDataType<bool>();
    auto isEqual = (datatype1 == dataype2); //returns false

    auto test = server.m_server->getNode({ 1, "Test" }).readValueScalar<bool>();
    return 0;
}

serverWrapper.hpp

#pragma once
#ifdef WRAPPER_LIBRARY
#define WRAPPER_EXPORT __declspec(dllimport)
#else
#define WRAPPER_EXPORT __declspec(dllexport)
#endif

#include <open62541pp/open62541pp.h>

class WRAPPER_EXPORT serverWrapper
{
public:
    ::opcua::Server* m_server = new ::opcua::Server();
    serverWrapper();

    bool executeInOneTranslationUnit();
};

serverWrapper.cpp

#include "serverWrapper.hpp"

serverWrapper::serverWrapper()
{
    auto objectsNode = m_server->getObjectsNode();
    auto onlineNode = objectsNode.addObject({ 1, "MyObject" }, "MyObject");
    auto myNode = onlineNode.addVariable({ 1, "Test" }, "TheAnswer");
    myNode.writeValue(::opcua::Variant::fromScalar(true));
}

bool serverWrapper::executeInOneTranslationUnit()
{
    auto asVariant = m_server->getNode({ 1, "Test" }).readValue();
    auto datatype1 = asVariant.getDataType();
    auto dataype2 = &opcua::getDataType<bool>();
    auto isEqual = (datatype1 == dataype2);
    return m_server->getNode({ 1, "Test" }).readValueScalar<bool>();
}

I think this regression was introduced by commit 43f4c392a9f8e35b7aa8b469c8bf8be7d9a5b70c

lukasberbuer commented 6 months ago

Thanks @pemartin2 for spotting this and the detailed analysis. You are right, UA_DataTypes should be compared by their typeId. I just prepared a fix in #252. Can you verify if this works?

pemartin2 commented 6 months ago

Hi @lukasberbuer. Thank you very much for the quick fix. I am a bit busy at the moment but will check as soon as I find some time.

lukasberbuer commented 6 months ago

Should be fixed with #252. Please reopen or comment if there are any remaining issues.

pemartin2 commented 5 months ago

Confirmed to be fixed in my project's context.