Code indexing in gitaly is broken and leads to code not being visible to the user. We work on the issue with highest priority.

Skip to content
Snippets Groups Projects

Don't use GLIBCXX_ASSERTIONS in production

Merged Don't use GLIBCXX_ASSERTIONS in production
627-add-wp-d_glibcxx_assertions-to-the-build-flags-3 into master
2 unresolved threads
Merged kraus requested to merge 627-add-wp-d_glibcxx_assertions-to-the-build-flags-3 into master
2 unresolved threads

Closes #627 (closed)

I assumed that this flag GLIBCXX_ASSERTIONS has no effect when the build type is Release. However I did some timing measurements and observed an increase of ~6 percent when using. This is fine for development builds but not for production.

Edited by kraus

Merge request reports

Merged by krauskraus 4 years ago (Feb 28, 2021 4:12pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • snuverink_j resolved all threads

    resolved all threads

  • snuverink_j approved this merge request

    approved this merge request

  • kraus mentioned in merge request !471 (merged)

    mentioned in merge request !471 (merged)

  • kraus
    kraus @kraus ·
    Author Developer

    @gsell could you review this MR?

  • gsell
    gsell @gsell started a thread on the diff
    • CMakeLists.txt
    101 101
    102 102 # Enables extra error checking in the form of precondition assertion, such
    103 103 # as bounds checking and null pointer checks when dereferencing smart pointers
    104 add_compile_options(-Wp,-D_GLIBCXX_ASSERTIONS)
    104 string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
    105 if (uppercase_CMAKE_BUILD_TYPE MATCHES "^(DEBUG|RELWITHDEBINFO)$")
    106 add_compile_options(-Wp,-D_GLIBCXX_ASSERTIONS)
    107 endif()
    • gsell
      gsell @gsell ·
      Owner

      @kraus sorry, did not saw this MR.

      Actually I would prefer to have this option in for all build types as long as the code has not been released. Hence in the master it should be in for all build types.

    • kraus
      kraus @kraus ·
      Author Developer

      Ok, that's fine for me. However we then should extend the description of the procedure to release new versions such that you (or who ever prepares a new release) don't forget to remove this flag.

    • gsell
      gsell @gsell ·
      Owner

      We can use the version number or another mechanism for this.

    • kraus
      kraus @kraus ·
      Author Developer

      Could you make a more discreet proposition? E.g. always off when minor version is even?

    • gsell
      gsell @gsell ·
      Owner

      I see these two options:

      1. minor version is even
      2. a variable set in the header of the main CMakeLists.txt. This has the advantage of being more independent from the version numbers - just in case we want to change the numbering scheme.
    • kraus
      kraus @kraus ·
      Author Developer

      Ok, I've opted for the second proposition.

    • Please register or sign in to reply
  • kraus added 1 commit

    added 1 commit

    • 197cf862 - don't disable glibcxx_assertions when the build type is Release but if the...

    Compare with previous version

  • snuverink_j
    snuverink_j @snuverink_j started a thread on an outdated change in commit 197cf862
    • CMakeLists.txt
    1 1 cmake_minimum_required (VERSION 3.1)
    2 2 project (OPAL VERSION 2.5.0)
    3 3 set (PROJECT_BUGREPORT opal@lists.psi.ch)
    4 set (BUILD_FOR_PRODUCTION OFF)
  • kraus added 2 commits

    added 2 commits

    • 792efb66 - Revert "don't disable glibcxx_assertions when the build type is Release but if...
    • 2600cf54 - use option BUILD_FOR_PRODUCTION to switch on and off assertions

    Compare with previous version

  • snuverink_j approved this merge request

    approved this merge request

  • kraus
    kraus @kraus ·
    Author Developer

    @gsell could you have a look at this MR?

  • gsell approved this merge request

    approved this merge request

  • kraus merged

    merged

  • kraus mentioned in commit b02ea9bf

    mentioned in commit b02ea9bf

  • ext-calvo_p changed milestone to %OPAL 2021.1

    changed milestone to %OPAL 2021.1

    • Please register or sign in to reply