LMDB guard for CTP7 modules migration - Githubissues

cms-gem-daq-project / xhal

XHAL interface library

0 stars 10 forks source link

LMDB guard for CTP7 modules migration #131

Closed lmoureaux closed 5 years ago

lmoureaux commented 5 years ago

Description

This PR adds a guard that abstracts away the management of the register address database. The current implementation doesn't try to be smart and does the same as the current localArgs initialization.

The new code cannot be compiled into libxhal.so for the host PC (unless we want to carry LMDB as a dependency), therefore I put the files in the xhalarm package structure. However I can't find a way to include them into the build without duplicating most of the Makefile; @jsturdy maybe you can suggest something? I think the canonical way of doing this is to build libxhalcore.so for two architectures from the same code and Makefile (with CPP, CC, CXX, LD etc passed from a root Makefile using the environment), and to build libxhalarm.so for ARM, with only the CTP7-specific code.

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Motivation and Context

Required to port away from localArgs.

How Has This Been Tested?

Compiles and links on x86 with CMake. More testing is needed on a real CTP7, but build system integration is required before this can happen.

Checklist:

[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[x] I have updated the documentation accordingly.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

lpetre-ulb commented 5 years ago

The new code cannot be compiled into libxhal.so for the host PC (unless we want to carry LMDB as a dependency), therefore I put the files in the xhalarm package structure. However I can't find a way to include them into the build without duplicating most of the Makefile; @jsturdy maybe you can suggest something? I think the canonical way of doing this is to build libxhalcore.so for two architectures from the same code and Makefile (with CPP, CC, CXX, LD etc passed from a root Makefile using the environment), and to build libxhalarm.so for ARM, with only the CTP7-specific code.

It seems the header file is also inside xhalarm. How does the header file gets installed on the development machine and how is it included in the client application (during compilation only since RPC methods can (will?) inherit from xhal::LMDBGuard)? It is installed in the same location as the xhalcore header files?

To me, what's happening with the xhal library is very strange. We are getting two libraries with the same name, but which provide different features and symbols depending on the target. Shouldn't we try to rationalize and create one library per functionality (e.g. libhal_rpc.so, 'libxhal_lmdb.so,libxhal_utils.so`, ...) and compile the libraries only for the supported targets? Features and symbols would be the same, even though the implementation can differ.

jsturdy commented 5 years ago

The new code cannot be compiled into libxhal.so for the host PC (unless we want to carry LMDB as a dependency), therefore I put the files in the xhalarm package structure. However I can't find a way to include them into the build without duplicating most of the Makefile; @jsturdy maybe you can suggest something? I think the canonical way of doing this is to build libxhalcore.so for two architectures from the same code and Makefile (with CPP, CC, CXX, LD etc passed from a root Makefile using the environment), and to build libxhalarm.so for ARM, with only the CTP7-specific code.

It seems the header file is also inside xhalarm. How does the header file gets installed on the development machine and how is it included in the client application (during compilation only since RPC methods can (will?) inherit from xhal::LMDBGuard)? It is installed in the same location as the xhalcore header files?

Currently, the MO has been that if we're developing applications that depend on, e.g., xhal, whether for arm or for x86_64, we need headers, and if for arm the arm compatible libraries for anything that needs to be compiled for the arm architecture. So during the RPM packaging stage, the arm compatible libs (and eventually arm specific headers, though there hasn't yet been such a package) are copied into the RPMBUILD tree for packaging, such that they can be installed with the xhal-devel package. We could do the equivalent of putting the arm libs into lib/arm for the headers with an include/xhal/arm or include/arm/xhal (probably the second makes the code more adaptable)

What we could (should?) do, is somehow create our own PETA_STAGE images, (or an extension package), which can be updated with the devel packages that we're producing, as this also helps to keep the wall between arm and x86_64

To me, what's happening with the xhal library is very strange. We are getting two libraries with the same name, but which provide different features and symbols depending on the target. Shouldn't we try to rationalize and create one library per functionality (e.g. libhal_rpc.so, 'libxhal_lmdb.so,libxhal_utils.so`, ...) and compile the libraries only for the supported targets? Features and symbols would be the same, even though the implementation can differ.

Yes, this is probably a better route for this package, since its origins in being PC based, are now migrating to a dual use library, with distinct features on each side of the RPC call. Let's discuss this in #132

jsturdy commented 5 years ago

Tried an attempt to package as being discussed in #132, but the code doesn't compile with the Zynq compiler (arm-linux-gnueabihf-gcc -dumpversion: 4.9.2) and the following error message:

arm-linux-gnueabihf-gcc -fomit-frame-pointer -pipe -fno-common -fno-builtin -Wall -std=c++14 -march=armv7-a -mfpu=neon -mfloat-abi=hard -mthumb-interwork -mtune=cortex-a9 -DEMBED -Dlinux -D__linux__ -Dunix -fPIC --sysroot=/data/bigdisk/sw/peta-stage -I/data/bigdisk/sw/peta-stage/usr/include -I/data/bigdisk/sw/peta-stage/include -g -std=gnu++14 -I/data/bigdisk/use
rs/sturdy/software/xhal/xhal/include/common -I/data/bigdisk/users/sturdy/software/xhal/xhal/include/arm -c -MT /data/bigdisk/users/sturdy/software/xhal/xhalarm/src/linux/arm/arm/LMDB.o -MMD -MP -MF /data/bigdisk/users/sturdy/software/xhal/xhalarm/src/linux/arm/arm/LMDB.Td -o /data/bigdisk/users/sturdy/software/xhal/xhalarm/src/linux/arm/arm/LMDB.o /data/bigdisk/u
sers/sturdy/software/xhal/xhal/src/arm/LMDB.cpp
In file included from /data/bigdisk/users/sturdy/software/xhal/xhal/src/arm/LMDB.cpp:1:0:
/data/bigdisk/users/sturdy/software/xhal/xhal/include/arm/xhal/LMDB.h:60:30: error: defaulted declaration ‘constexpr xhal::LMDBGuard& xhal::LMDBGuard::operator=(xhal::LMDBGuard&&) const’
         constexpr LMDBGuard &operator=(LMDBGuard &&) = default;
                              ^
/data/bigdisk/users/sturdy/software/xhal/xhal/include/arm/xhal/LMDB.h:60:30: error: does not match expected signature ‘xhal::LMDBGuard& xhal::LMDBGuard::operator=(xhal::LMDBGuard&&)’
/data/bigdisk/users/sturdy/software/xhal/xhal/include/arm/xhal/LMDB.h:60:30: error: explicitly defaulted function ‘constexpr xhal::LMDBGuard& xhal::LMDBGuard::operator=(xhal::LMDBGuard&&) const’ cannot be declared as constexpr because the implicit declaration is not constexpr:
/data/bigdisk/users/sturdy/software/xhal/xhal/src/arm/LMDB.cpp: In constructor ‘xhal::{anonymous}::Singleton::Singleton()’:
/data/bigdisk/users/sturdy/software/xhal/xhal/src/arm/LMDB.cpp:43:23: warning: ‘xhal::{anonymous}::Singleton::rtxn’ will be initialized after [-Wreorder]
             lmdb::txn rtxn; ///< \brief Read-only transaction
                       ^
/data/bigdisk/users/sturdy/software/xhal/xhal/src/arm/LMDB.cpp:42:23: warning:   ‘lmdb::dbi xhal::{anonymous}::Singleton::dbi’ [-Wreorder]
             lmdb::dbi dbi;  ///< \brief Database handle
                       ^
/data/bigdisk/users/sturdy/software/xhal/xhal/src/arm/LMDB.cpp:50:13: warning:   when initialized here [-Wreorder]
             Singleton() : // NOTE: Order is important!
             ^
make[1]: *** [/data/bigdisk/users/sturdy/software/xhal/xhalarm/src/linux/arm/arm/LMDB.o] Error 1
make[1]: Leaving directory `/data/bigdisk/users/sturdy/software/xhal/xhalarm'
make: *** [xhalarm] Error 2

Removing the constexpr allows the compilation to complete... so I can at least continue to poke around with the packaging, but of course best to let the author correct this as intended :-)

jsturdy commented 5 years ago

Also, can't compile for "client" because of lack of std::make_unique in gcc-4.8.5... workaround can be made, so continuing testing only the packaging, and can open a separate issue about how to handle this.

lmoureaux commented 5 years ago

Should now compile without errors or warnings.

jsturdy commented 5 years ago

@mexanick @lmoureaux, any reason to not merge this (as mentioned in https://github.com/cms-gem-daq-project/xhal/issues/132#issuecomment-528561345?

lmoureaux commented 5 years ago

I'm reluctant to merging something that doesn't build out-of-the-box. Maybe I can move it to xhalcore for the time being? (Since the repo structure will be overhauled anyway)

jsturdy commented 5 years ago

I'm reluctant to merging something that doesn't build out-of-the-box. Maybe I can move it to xhalcore for the time being? (Since the repo structure will be overhauled anyway)

Sounds fine to me (I can confirm that it builds after my moving things around though I haven't tested that the library works...)

lmoureaux commented 5 years ago

clang-tidy finding:

xhal/xhalarm/src/common/LMDB.cpp:32:13: warning: Address of stack memory associated with local variable 'env' returned to caller [clang-analyzer-core.StackAddressEscape]
            return std::move(env);
            ^
xhal/xhalarm/src/common/LMDB.cpp:51:21: note: Calling 'create_env'
                env(create_env()),
                    ^
xhal/xhalarm/src/common/LMDB.cpp:25:17: note: Assuming the condition is false
            if (path == nullptr) {
                ^
xhal/xhalarm/src/common/LMDB.cpp:25:13: note: Taking false branch
            if (path == nullptr) {
            ^
xhal/xhalarm/src/common/LMDB.cpp:32:13: note: Address of stack memory associated with local variable 'env' returned to caller
            return std::move(env);
            ^
xhal/xhalarm/src/common/LMDB.cpp:81:16: warning: function previously declared with an explicit exception specification redeclared with an implicit exception specification [clang-diagnostic-implicit-exception-spec-mismatch]
    LMDBGuard::~LMDBGuard()
               ^
xhal/xhalarm/include/xhal/LMDB.h:66:9: note: previous declaration is here
        ~LMDBGuard() noexcept;
        ^

lmoureaux commented 5 years ago

Fixed clang-tidy warnings, moved to xhalcore.