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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 14, 2024
+36 −19
Merged

Add [[noreturn]] attribute to Output::fatal, add SST_Exit() function #1175

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
923e5b9
Add [[noreturn]] attribute to Output::fatal, add SST_Exit() function
leekillough Nov 9, 2024
1839d2d
Move SST_Exit() to simulation_impl.h and simulation.cc
leekillough Nov 13, 2024
0068599
Move SST_Exit() declaration to top of file since there a references t…
leekillough Nov 13, 2024
019b6f4
fix #include
leekillough Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/sst/core/exit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "sst_config.h"

#include "sst/core/exit.h"
#include <exception>
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.


#include "sst/core/warnmacros.h"
#ifdef SST_CONFIG_HAVE_MPI
Expand Down Expand Up @@ -206,4 +207,23 @@ Exit::serialize_order(SST::Core::Serialization::serializer& ser)
ser& single_rank;
}

[[noreturn]] void
SST_Exit(int exit_code)
{
// Make sure only one thread calls MPI_Abort() or exit() in the
// case where two threads call fatal() at the same time
// Only one thread initializes the function-local static variable
// Other threads are blocked

#ifdef SST_CONFIG_HAVE_MPI
// If MPI exists, abort
static int exit_once = (MPI_Abort(MPI_COMM_WORLD, exit_code), 0);
#else
static int exit_once = (exit(exit_code), 0);
#endif

// Should never get here
std::terminate();
}

} // namespace SST
2 changes: 2 additions & 0 deletions src/sst/core/exit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ class Exit : public Action
bool single_rank;
};

[[noreturn]] void SST_Exit(int exit_code);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.


} // namespace SST

#endif // SST_CORE_EXIT_H
19 changes: 3 additions & 16 deletions src/sst/core/output.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "sst/core/output.h"

// Core Headers
#include "sst/core/exit.h"
#include "sst/core/simulation_impl.h"
#include "sst/core/warnmacros.h"

Expand All @@ -41,9 +42,6 @@ REENABLE_WARNING

namespace SST {

// Atomic to control access to calling MPI_Abort or exit() in fatal() call
std::atomic<int> fatal_count = 0;

// Initialize The Static Member Variables
Output Output::m_defaultObject;
std::string Output::m_sstGlobalSimFileName = "";
Expand Down Expand Up @@ -159,7 +157,7 @@ Output::getOutputLocation() const
return m_targetLoc;
}

void
[[noreturn]] void
Output::fatal(uint32_t line, const char* file, const char* func, int exit_code, const char* format, ...) const
{
va_list arg1;
Expand Down Expand Up @@ -217,18 +215,7 @@ Output::fatal(uint32_t line, const char* file, const char* func, int exit_code,

Simulation_impl::emergencyShutdown();

int count = fatal_count.fetch_add(1);

// Make sure only one thread calls MPI_Abort() or exit() in the
// case where two threads call fatal() at the same time
if ( count == 0 ) {
#ifdef SST_CONFIG_HAVE_MPI
// If MPI exists, abort
MPI_Abort(MPI_COMM_WORLD, exit_code);
#else
exit(exit_code);
#endif
}
SST_Exit(exit_code);
}

void
Expand Down
3 changes: 2 additions & 1 deletion src/sst/core/output.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,8 @@ class Output
@param format Format string. All valid formats for printf are available.
@param ... Arguments for format.
*/
void fatal(uint32_t line, const char* file, const char* func, int exit_code, const char* format, ...) const
[[noreturn]] void
fatal(uint32_t line, const char* file, const char* func, int exit_code, const char* format, ...) const
__attribute__((format(printf, 6, 7)));

// GET / SET METHODS
Expand Down
5 changes: 3 additions & 2 deletions src/sst/core/simulation_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "sst/core/clock.h"
#include "sst/core/componentInfo.h"
#include "sst/core/exit.h"
#include "sst/core/oneshot.h"
#include "sst/core/output.h"
#include "sst/core/profile/profiletool.h"
Expand Down Expand Up @@ -243,7 +244,7 @@ class Simulation_impl
if ( nullptr != i ) { return i->getComponent(); }
else {
printf("Simulation::getComponent() couldn't find component with id = %" PRIu64 "\n", id);
exit(1);
SST_Exit(1);
}
}

Expand All @@ -255,7 +256,7 @@ class Simulation_impl
if ( nullptr != i ) { return i; }
else {
printf("Simulation::getComponentInfo() couldn't find component with id = %" PRIu64 "\n", id);
exit(1);
SST_Exit(1);
}
}

Expand Down
Loading