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

fix: json gcc runtime error #1301

Merged
merged 4 commits into from
Jul 6, 2022

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Jul 6, 2022

I was facing a runtime error at https://github.com/acts-project/acts/blob/main/Plugins/Json/include/Acts/Plugins/Json/GeometryHierarchyMapJsonConverter.hpp#L110

after applying the changes everything is back to normal

looks like a problem with gcc and nlohmann/json?

gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super strange. This is fine to go in obviously, but can you give it a quick try with static constexpr std::string_view by any chance?

@andiwand
Copy link
Contributor Author

andiwand commented Jul 6, 2022

the static constexpr const char* change was actually not critical but I left it in because it think it is more correct?

first I was not even able to print kHeaderKey for example

the critical change was unrolling the initialization list but I really don't know why it caused a runtime error

@paulgessinger paulgessinger added this to the next milestone Jul 6, 2022
@HadrienG2
Copy link
Contributor

HadrienG2 commented Jul 6, 2022

Note that compiler runtime errors are almost always bugs (barring edge cases like your computer running out of RAM) and should be reported as such to the compiler developers : https://gcc.gnu.org/bugs/

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #1301 (747c0e7) into main (d3bddc9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1301   +/-   ##
=======================================
  Coverage   47.42%   47.42%           
=======================================
  Files         375      375           
  Lines       19788    19788           
  Branches     9287     9287           
=======================================
  Hits         9385     9385           
  Misses       4021     4021           
  Partials     6382     6382           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@andiwand
Copy link
Contributor Author

andiwand commented Jul 6, 2022

@HadrienG2 I thought it could be due to a bug in the libraryl like UB that turns out okay with clang but not gcc. or they use a language "feature" that does not exist in the standard. not sure how likely those cases are compared to each other tho

@HadrienG2
Copy link
Contributor

HadrienG2 commented Jul 6, 2022

@HadrienG2 I thought it could be due to a bug in the libraryl like UB that turns out okay with clang but not gcc. or they use a language "feature" that does not exist in the standard. not sure how likely those cases are compared to each other tho

In this case, the compiler should emit a standard error or warning message following the usual style, like this:

If on the other hand you get an internal compiler error message like the last lines of this:

...then it means the compiler has crashed in a way that was not expected by its developers (hence the absence of a proper error message following the usual style). These errors are the ones that should be reported.

@andiwand
Copy link
Contributor Author

andiwand commented Jul 6, 2022

@HadrienG2 sorry I think I messed things up with my poor PR title. there was not a runtime error while compiling with gcc but a runtime error unfolded when executing Acts compiling with gcc while it works fine with clang

@HadrienG2
Copy link
Contributor

HadrienG2 commented Jul 6, 2022

Ah, sorry for misinterpreting on my side as well.

A simple check that is quite effective at discriminating between UB and other issues is to rebuild without optimizations (CMake Debug build or -O0 manual optimization flag). If the runtime error goes away in this configuration, then it's likely to be a UB problem.

@kodiakhq kodiakhq bot merged commit 695d9e2 into acts-project:main Jul 6, 2022
@HadrienG2
Copy link
Contributor

HadrienG2 commented Jul 6, 2022

...and if the bug is still there at -O0, then you can build with debug symbols (-g, also part of CMake's Debug build), and use GDB to locate the precise point at which the program is crashing, which is often quite helpful.

@andiwand andiwand deleted the fix-json-gcc-runtime-error branch July 6, 2022 11:44
@andiwand andiwand modified the milestones: next, v19.4.0 Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants