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

Linking to OusterSDK::ouster_client crashes our program due to EIGEN_MAX_ALIGN_BYTES=32 #396

Closed
vebjornjr opened this issue Jun 29, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@vebjornjr
Copy link

Hello.

I have worked with your library in a separate little test project until now.
Today I tried to use your library and my Ouster driver code in our main project, however this causes problems.
It causes our program to crash with a signal 11 (SIGSEGV).
I managed to reproduce it without actually using any code in your library, but only linking to it with target_link_libraries().
Further, I narrowed it down to the compiler define -DEIGEN_MAX_ALIGN_BYTES=32.
Simply using that compiler defined, even without linking to your library, crashes our program.

Do you happen to have any idea why that may be? And why that compiler define is needed?

linux-vdso.so.1 : __kernel_rt_sigreturn()+0
/lib/aarch64-linux-gnu/libc.so.6 : free()+0x24

OS: Ubuntu 18.04
Compiler: GCC 7.5.0
Nvidia Jetson AGX Xavier Development Kit

@vebjornjr
Copy link
Author

I did some further investigation.

I see that in Eigen documentation, the value of EIGEN_MAX_ALIGN_BYTES is set to a default value automatically based architecture, compiler, and OS if not provided.
In our program I see that it is automatically set to 16 by printing EIGEN_MAX_ALIGN_BYTES (which we do not specifically set).

So it seems like setting it to 32 instead of the automatic 16 on our system breaks our application. The question on why you set that compiler define still remains.

@kairenw
Copy link
Contributor

kairenw commented Jul 1, 2022

Mind showing us how you've set up your CMakeLists.txt ? We tried to write it so that it would only set EIGEN_MAX_ALIGN_BYTES if ouster_example was the top level

@vebjornjr
Copy link
Author

We build the library (ouster_client.a) the following way:

cd ouster_example-20220608 && mkdir build && cd build

ccmake .. -G Ninja

And changes the following options (rest as default):

BUILD_VIZ: OFF
CMAKE_BUILD_TYPE: Release
CMAKE_INSTALL_PREFIX: /PATH_TO_OUR_GIT_FOLDER/external

cmake --build .

cmake --install .

Our top level project (stripped away unnecessary info and changed some naming):

cmake_minimum_required(VERSION 3.16)

project(CompanyTopLevelProject
    VERSION 1.0
    LANGUAGES CXX
)

## Common compiler/linker settings ##

# Use C++17
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

# Add the external libraries folder to all search paths
set(CMAKE_PREFIX_PATH ${CMAKE_SOURCE_DIR}/../../external)

## Subprojects ##

add_subdirectory(CompanySubProject)

Our sub project:

cmake_minimum_required(VERSION 3.16)

project(CompanySubProject
    VERSION 1.0
    LANGUAGES CXX
)


## Packages ##

find_package(OusterSDK 20220608 EXACT CONFIG REQUIRED)


add_library(${PROJECT_NAME} SHARED)

target_include_directories(${PROJECT_NAME}
    PUBLIC
        ${PROJECT_SOURCE_DIR}
)

target_link_libraries(${PROJECT_NAME}
    PUBLIC
        Eigen3::Eigen
        OusterSDK::ouster_client
)

target_sources(${PROJECT_NAME}
    PRIVATE
        sourcefile1.cpp
        sourcefile2.cpp
)

I don't think it works like you intend it to. Because when I look at the file external/lib/cmake/OusterSDK/OusterSDKTargets.cmake I see the following:

set_target_properties(OusterSDK::ouster_client PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "EIGEN_MAX_ALIGN_BYTES=32"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include/optional-lite;${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "Eigen3::Eigen;\$<LINK_ONLY:jsoncpp_lib>"
  INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "include/optional-lite"
)

@vebjornjr
Copy link
Author

I think you want use the BUILD_INTERFACE generator expression to set EIGEN_MAX_ALIGN_BYTES. It is described here and here.

@vebjornjr
Copy link
Author

Never mind, that did not work as I indented. I tried to use BUILD_INTERFACE generator expression to only set the define during building (and not when linking to the installed library), but this still causes crashes since the library is compiled with a different value of the define compared to the value that is set automatically for us.

@kairenw
Copy link
Contributor

kairenw commented Jul 2, 2022

Is it possible you're linking against another static pre-built library that uses Eigen, overriding EIGEN_MAX_ALIGN_BYTES back to 16? The way we've set up our OusterSDKConfig.cmake, EIGEN_MAX_ALIGN_BYTES ought to propagate through to your code such that printing it should give you 32.

@vebjornjr
Copy link
Author

No, you seem to misunderstand.

When I don't link to OusterSDK:

  • Printing EIGEN_MAX_ALIGN_BYTES gives 16.
  • Looking at the compiler output, I see that no libraries set the compiler define.
  • So EIGEN_MAX_ALIGN_BYTES is automatically set to 16 on our platform.

When I do not make any changes to the OusterSDK, but links to it:

  • Printing EIGEN_MAX_ALIGN_BYTES gives 32.
  • I see in the compiler output that the compiler define (-DEIGEN_MAX_ALIGN_BYTES=32) is set by your library.
  • Our program crashes.

When I make changes to the CMakeLists in OusterSDK by removing the lines that set EIGEN_MAX_ALIGN_BYTES:

  • Printing EIGEN_MAX_ALIGN_BYTES gives 16.
  • Looking at the compiler output, I see that the compiler define it not set.
  • Everything works.

So why do want to force EIGEN_MAX_ALIGN_BYTES to the value 32? Why can't it be the automatically computed value? Or why can't you provide a build option so we can at least set it to the value that works for us.

@kairenw
Copy link
Contributor

kairenw commented Jul 4, 2022

Yes, sorry. I do understand your problem, or have a good guess at it. The problem is likely that another pre-built library you're using which does not explicitly set EIGEN_MAX_ALIGN_BYTES defaults to 16 when it compiles. Then once you link against ours and theirs and start passing Eigen datastructures between libraries, it starts mixing the aligned and non-aligned versions of free for some bad news! Overriding was not the best way to put that.

The reason we set it initially was to enable avx usage, which requires 32 for alignment. Obviously since you're on a Jetson Xavier, avx does not apply for you.

We're hoping to approve #371, which makes it a build option, once it they make a small change with the names. In the meantime, as you've discovered, it may be easiest to simply remove the line from the CMakeLists on your own build.

@kairenw kairenw added the enhancement New feature or request label Jul 5, 2022
@kairenw
Copy link
Contributor

kairenw commented Aug 24, 2022

This should be addressed by the addition of the option OUSTER_USE_EIGEN_MAX_ALIGN_BYTES_32 in the newest push to GH located here. Thank you for your patience on this. Let us know if that doesn't work for you. Closing this out now.

@kairenw kairenw closed this as completed Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants