-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add [[noreturn]] attribute to Output::fatal, add SST_Exit() function #1175
base: devel
Are you sure you want to change the base?
Conversation
CLANG-FORMAT TEST - FAILED (on last commit): |
CMAKE-FORMAT TEST - PASSED |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
CLANG-FORMAT TEST - FAILED (on last commit): |
CMAKE-FORMAT TEST - PASSED |
CLANG-FORMAT TEST - FAILED (on last commit): |
CMAKE-FORMAT TEST - PASSED |
CLANG-FORMAT TEST - FAILED (on last commit): |
CMAKE-FORMAT TEST - PASSED |
CMAKE-FORMAT TEST - PASSED |
CLANG-FORMAT TEST - FAILED (on last commit): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together. In general, things look good. Just need to address the clang-format errors and we can get it released for testing.
src/sst/core/exit.cc
Outdated
@@ -12,6 +12,7 @@ | |||
#include "sst_config.h" | |||
|
|||
#include "sst/core/exit.h" | |||
#include <exception> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the coding standards, includes of standard headers should go after all sst includes. This is probably contributing to the clang-format failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran clang-format
but my version is newer than v12 and I didn't feel like trying to go back to an older version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not promising for moving clang-format forward if it doesn't follow the header ordering settings in the .clang-format file. You can look at the diff of what it got with what it wanted in the "checks" tab in the PR. Just select the clang-format check and click in the Matrix box to bring up the results of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's too late to fix it now, but clang-format
should have a version 1.2.3
directive for .clang-format
files so that it behaves like a certain version. But as the number of versions increases, this becomes hard to test, and complicates the code to behave differently for different versions, although if this versioning scheme is done from the beginning, the versioned tests can be developed incrementally.
src/sst/core/exit.h
Outdated
@@ -117,6 +117,8 @@ class Exit : public Action | |||
bool single_rank; | |||
}; | |||
|
|||
[[noreturn]] void SST_Exit(int exit_code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if exit.h is the best place for this function to live. One of the issues is that exit.h is being installed, though it really shouldn't be, and SST_Exit() shouldn't be part of the public API. The other issue is that even though the names match, the functionality isn't really related to the Exit object. Of the header files that aren't installed, I think simulation_impl.h is probably the best place for the SST_Exit() function (not as part of the Simulation_impl object, however). Since The function won't be part of the non-core API, I think it's fine to leave as is for now if you don't want to change it and we can reevaluate later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I can move it anywhere, to prevent it from being publicly exported / documented. The exit.h
and exit.cc
seemed logical, even if this function is unrelated to the Exit object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and since Exit
is a common name which might conflict with other uses, I used SST::SST_Exit
instead of SST::Exit
. If this deviates from normal SST naming conventions, such as case and underscores, it can be renamed. In Rev we use camelCase
for variables / functions and CamelCase
for types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to prefix it with SST. We haven't been good about enforcing naming conventions, though nominally we match the same camel case you describe. That being said, I think I like SST_Exit() better than SSTExit(). We can reevaluate later when we take a pass at cleaning up other names in the core.
CLANG-FORMAT TEST - PASSED |
CMAKE-FORMAT TEST - PASSED |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore
Using Repos:
Pull Request Author: leekillough |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist
Job: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core
Job: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements
Job: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore
|
…o it in the header
CMAKE-FORMAT TEST - PASSED |
CLANG-FORMAT TEST - PASSED |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore
Using Repos:
Pull Request Author: leekillough |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist
Job: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core
Job: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements
Job: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore
|
I'll investigate the failure. Maybe the I don't mind taking this slowly. I want to make sure it is safe. |
Just looked at the log. It appears that mpi.h is not included in simulation.cc. |
Should we conditionally include |
Yes. We generally do this for including mpi.h: #ifdef SST_CONFIG_HAVE_MPI The extra macros are to suppress some of the known warnings in openmpi's version of mpi.h. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist
Build InformationTest Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements
Build InformationTest Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore
Using Repos:
Pull Request Author: leekillough |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore
Job: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist
Job: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core
Job: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements
Job: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore
|
Fixes #1169 by adding
[[noreturn]]
toSST::Output::fatal()
.Adds a
SST::SST_Exit()
function which ensures only a single thread exits if multiple threads call it simultaneously.