Warn in ScopeGuard about illegal usage #5250

crtrott · 2022-07-21T21:35:18Z

We (Damien and I) are considering a change of behavior and how to use it for ScopeGuard.
Specifically, we believe its current design is problematic in various ways, one of them being the silent ignoring of arguments handed to it if Kokkos was already initialized.

Besides better warnings in cases which were previously already broken (like creating a ScopeGuard after Kokkos::finalize() was called), the primary change is that you can't create a ScopeGuard after Kokkos::initialize was called. This also means that you can't create a ScopeGuard after any other ScopeGuard was created. The error message provided shows a way how to do nested scopeguards (as they might currently be in use) if really desired, but essentially its pushing more of the correctness check behavior on the user.

Please let us know what you think.

Note: this PR is missing tests, which I am working on.

core/src/Kokkos_Core.hpp

dalg24 · 2022-07-21T23:53:50Z

core/src/Kokkos_Core.hpp

 class KOKKOS_ATTRIBUTE_NODISCARD ScopeGuard {
 public:
 #if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard) >= 201907
  KOKKOS_ATTRIBUTE_NODISCARD
 #endif
  ScopeGuard(int& argc, char* argv[]) {
    sg_init = false;
+#ifdef KOKKOS_ENABLE_DEPRECATION_WARNINGS
+    if (is_initialized()) {


Suggested change

if (is_initialized()) {

if (is_initialized() || is_finalized()) {

I hope you will appreciate the irony here. Technically the code you propose here has a bug that the change I suggest above fixes. With the definition proposed in #5245 that would have be right.

If you are trying to only emit one of the warning your code should read

if (is_finalized()) { std::cerr << Impl::scopeguard_create_after_finalize_warning() << std::endl; } else if (is_initialized() || is_finalized()) { // obviously the || is_finallized() can be discarded // but I can't believe you can write that code and not see // I was right and that the option 2 in #5208 is more logical std::cerr << Impl::scopeguard_create_after_initialize_warning() << std::endl; }

core/src/Kokkos_Core.hpp

dalg24

I like the use of function to produce the strings and the general organization besides the comments I made

dalg24 · 2022-07-22T00:36:56Z

core/src/Kokkos_Core.hpp

+  ScopeGuard(int& argc, char* argv[]) {
+    if (is_initialized()) {
+      Kokkos::abort(Impl::scopeguard_create_after_init_warning().c_str());
+    }
+    if (is_finalized()) {
+      Kokkos::abort(Impl::scopeguard_create_after_finalize_warning().c_str());
+    }
+    initialize(argc, argv);
+  }


Commenting so I get a souvenir after Christian renames everything

dalg24

Looks good now.
Did you plan on adding tests?

masterleinad

Looks good to me but you still want to add tests, right?

masterleinad · 2022-07-22T12:33:50Z

core/src/Kokkos_Core.hpp

+#if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard) >= 201907
+  KOKKOS_ATTRIBUTE_NODISCARD
+#endif
+
+  ScopeGuard(int& argc, char* argv[]) {


Suggested change

#if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard) >= 201907

KOKKOS_ATTRIBUTE_NODISCARD

#endif

ScopeGuard(int& argc, char* argv[]) {

#if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard) >= 201907

KOKKOS_ATTRIBUTE_NODISCARD

#endif

ScopeGuard(int& argc, char* argv[]) {

masterleinad · 2022-07-22T12:35:21Z

core/src/Kokkos_Core.hpp

+    finalize();
+  }
+
+  // private:


Suggested change

// private:

masterleinad · 2022-07-22T12:35:54Z

core/src/Kokkos_Core.hpp

@@ -287,6 +393,7 @@ std::vector<ExecSpace> partition_space(ExecSpace space,

 // Specializations requires after core definitions


Suggested change

// Specializations requires after core definitions

// Specializations required after core definitions

?

nliber · 2022-07-22T15:50:52Z

core/src/Kokkos_Core.hpp

+#if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard) >= 201907
+  KOKKOS_ATTRIBUTE_NODISCARD
+#endif
+  ScopeGuard(


The single argument constructor should be explicit.

explicit ScopeGuard(const InitializationSettings& settings) { /* ... */ } ScopeGuard() : ScopeGuard(InitializationSettings()) {}

Note: I haven't quite thought through the implications if we end up keeping InitArguments around for a while as well, as that might require its own (possibly implicit) forwarding constructor for backwards compatibility.

Agreed on the explicit comment

Interaction with InitArguments will be fine https://godbolt.org/z/7scMhjMj6

Before we added InitializationSettings, the following three initialization syntaxes work:

InitArguments ia; ScopeGuard sc0 = ia; ScopeGuard sc1(ia); ScopeGuard sc2{ia};

If we only do explicit ScopeGuard(const InitializationSettings& settings) { /* ... */ }, sc0 no longer compiles.

For the record, I'm fine with breaking that. I'm just pointing out that it is a breaking change (albeit unrelated to the explicitness of the constructor).

If we don't want it to break, we need to add:

/* implicit */ ScopeGuard(const InitArguments& ia) : ScopeGuard(InitializationSettings(ia)) {}

https://godbolt.org/z/4d6ra7rh5

I tend to think it is fine to break the ScopeGuard sc0 = ia; use.

@crtrott you forgot to add explicit here. Considering that this is the new code path, I would have been fine with omitting at the other place but it must be added here.

nliber · 2022-07-22T15:52:47Z

core/src/Kokkos_Core.hpp

+
+  // private:
+  ScopeGuard& operator=(const ScopeGuard&) = delete;
+  ScopeGuard(const ScopeGuard&)            = delete;


While not strictly necessary, please add the other two special member functions (Rule of 5):

ScopeGuard& operator=(ScopeGuard&&) = delete; ScopeGuard(ScopeGuard&&) = delete;

crtrott · 2022-07-25T23:45:34Z

I won't have time to write tests for 3.7 here is a standalone manual test

#include <Kokkos_Core.hpp>
#include <cmath>

int main(int argc, char* argv[]) {
  {
    int create_after_init = argc > 1 ? atoi(argv[1]) : 0;
    int create_after_final = argc > 2 ? atoi(argv[2]) : 0;
    int create_after_other = argc > 3 ? atoi(argv[3]) : 0;
    int destroy_after_final = argc > 4 ? atoi(argv[4]) : 0;
    if(create_after_other) {
      Kokkos::ScopeGuard guard1(argc,argv);
      Kokkos::ScopeGuard guard2(argc,argv);

    } else {
      if(create_after_init || create_after_final)
        Kokkos::initialize(argc, argv);
      if(create_after_final)
        Kokkos::finalize();
      Kokkos::ScopeGuard guard(argc,argv);
      if((!create_after_final && create_after_init) || destroy_after_final)
        Kokkos::finalize();
    }
  }
}

Test results deprecated_code disabled is this, I also confirmed that I get that output with just deprecation warnings on.

[crtrott@kokkos-dev-2 ScopeGuard]$ ./test.cuda 0 0 0 0
[crtrott@kokkos-dev-2 ScopeGuard]$ ./test.cuda 1 0 0 0
Kokkos Error: Creating a ScopeGuard while Kokkos is initialized is illegal.
Do instead:
  std::unique_ptr<Kokkos::ScopeGuard> guard =
    !Kokkos::is_initialized() && !Kokkos::is_finalized()?
    new ScopeGuard(argc,argv) : nullptr;

Backtrace:
                        [0x447473]
                        [0x41b398]
                        [0x41b41b]
                        [0x407731]
                        [0x4058c6]
__libc_start_main [0x7fa3e7bcf555]
                        [0x406d79]
Aborted (core dumped)
[crtrott@kokkos-dev-2 ScopeGuard]$ ./test.cuda 0 1 0 0
Kokkos Error: Creating a ScopeGuard after Kokkos was finalized is illegal.
Do instead:
  std::unique_ptr<Kokkos::ScopeGuard> guard =
    !Kokkos::is_initialized() && !Kokkos::is_finalized()?
    new ScopeGuard(argc,argv) : nullptr;

Backtrace:
                        [0x447473]
                        [0x41b398]
                        [0x41b41b]
                        [0x407763]
                        [0x405895]
__libc_start_main [0x7fa271384555]
                        [0x406d79]
Aborted (core dumped)
[crtrott@kokkos-dev-2 ScopeGuard]$ ./test.cuda 0 0 1 0
Kokkos Error: Creating a ScopeGuard while Kokkos is initialized is illegal.
Do instead:
  std::unique_ptr<Kokkos::ScopeGuard> guard =
    !Kokkos::is_initialized() && !Kokkos::is_finalized()?
    new ScopeGuard(argc,argv) : nullptr;

Backtrace:
                        [0x447473]
                        [0x41b398]
                        [0x41b41b]
                        [0x407731]
                        [0x405965]
__libc_start_main [0x7f8351ae5555]
                        [0x406d79]
Aborted (core dumped)
[crtrott@kokkos-dev-2 ScopeGuard]$ ./test.cuda 0 0 0 1
Kokkos Error: Destroying a ScopeGuard after Kokkos was finalized is illegal.
Do instead:
  std::unique_ptr<Kokkos::ScopeGuard> guard =
    !Kokkos::is_initialized() && !Kokkos::is_finalized()?
    new ScopeGuard(argc,argv) : nullptr;

Backtrace:
                        [0x447473]
                        [0x41b398]
                        [0x41b41b]
                        [0x407798]
                        [0x4058a2]
__libc_start_main [0x7fb800433555]
                        [0x406d79]
Aborted (core dumped)

dalg24 · 2022-07-26T05:04:23Z

core/src/Kokkos_Core.hpp

@@ -334,9 +335,11 @@ class KOKKOS_ATTRIBUTE_NODISCARD ScopeGuard {
    finalize();
  }

-  // private:
+ private:


I am going to pretend I don't see that guy

dalg24 · 2022-07-26T12:03:56Z

core/src/Kokkos_Core.hpp

+      "Do instead:\n"
+      "  std::unique_ptr<Kokkos::ScopeGuard> guard =\n"
+      "    !Kokkos::is_initialized() && !Kokkos::is_finalized()?\n"
+      "    new ScopeGuard(argc,argv) : nullptr;\n");


I can live with it but I wanted to point out that this error will be raised even when the InitializationSettings argument was used.

masterleinad · 2022-07-26T14:21:54Z

core/src/Kokkos_Core.hpp

+  ScopeGuard& operator=(const ScopeGuard&) = delete;
+  ScopeGuard& operator=(ScopeGuard&&) = delete;
+  ScopeGuard(const ScopeGuard&)       = delete;
+  ScopeGuard(ScopeGuard&&)            = delete;


Why are these private?

because they were private before

or at least meant to be private??

It's more idiomatic not to have private copy constructors if you meant to delete them, right? This feels like part of a pre-C++11 approach where you couldn't use delete.

I agree with @masterleinad here.

Before C++11, the copy operations always existed for every class, so to make them noncopyable one made them private and unimplemented (or privately derived from something like boost::noncopyable, which did the same thing). Unimplemented always gave a linker error on an attempted copy, and making them private sometimes gave a compile time access error (depending on usage).

No need for that now that we can delete functions.

nliber · 2022-07-26T17:00:41Z

core/src/Kokkos_Core.hpp

+  ScopeGuard& operator=(const ScopeGuard&) = delete;
+  ScopeGuard& operator=(ScopeGuard&&) = delete;
+  ScopeGuard(const ScopeGuard&)       = delete;
+  ScopeGuard(ScopeGuard&&)            = delete;


I agree with @masterleinad here.

Before C++11, the copy operations always existed for every class, so to make them noncopyable one made them private and unimplemented (or privately derived from something like boost::noncopyable, which did the same thing). Unimplemented always gave a linker error on an attempted copy, and making them private sometimes gave a compile time access error (depending on usage).

No need for that now that we can delete functions.

crtrott · 2022-07-26T17:16:40Z

I removed the private ...

masterleinad

Thanks!

dalg24 · 2022-07-26T19:07:28Z

Failures were unrelated (machine running out of space)

Warn in ScopeGuard about illegal usage

6a21617

JBludau reviewed Jul 21, 2022

View reviewed changes

core/src/Kokkos_Core.hpp Outdated Show resolved Hide resolved

JBludau reviewed Jul 21, 2022

View reviewed changes

core/src/Kokkos_Core.hpp Show resolved Hide resolved

nmm0 reviewed Jul 21, 2022

View reviewed changes

core/src/Kokkos_Core.hpp Show resolved Hide resolved

core/src/Kokkos_Core.hpp Show resolved Hide resolved

core/src/Kokkos_Core.hpp Show resolved Hide resolved

dalg24 reviewed Jul 22, 2022

View reviewed changes

Address some comments

65e7f04

dalg24 approved these changes Jul 22, 2022

View reviewed changes

masterleinad reviewed Jul 22, 2022

View reviewed changes

nliber requested changes Jul 22, 2022

View reviewed changes

crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jul 25, 2022

Addressing review comments

d20687e

crtrott force-pushed the scopeguard branch from 945f245 to d20687e Compare July 26, 2022 02:31

dalg24 reviewed Jul 26, 2022

View reviewed changes

masterleinad reviewed Jul 26, 2022

View reviewed changes

nliber requested changes Jul 26, 2022

View reviewed changes

Removing the private in ScopeGuard

da45f52

masterleinad approved these changes Jul 26, 2022

View reviewed changes

nliber approved these changes Jul 26, 2022

View reviewed changes

JBludau approved these changes Jul 26, 2022

View reviewed changes

dalg24 merged commit 8e5ae2b into kokkos:develop Jul 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn in ScopeGuard about illegal usage #5250

Warn in ScopeGuard about illegal usage #5250

crtrott commented Jul 21, 2022

dalg24 Jul 21, 2022 •

edited

dalg24 left a comment

dalg24 Jul 22, 2022

dalg24 left a comment

masterleinad left a comment

masterleinad Jul 22, 2022

masterleinad Jul 22, 2022

masterleinad Jul 22, 2022

nliber Jul 22, 2022 •

edited

dalg24 Jul 22, 2022

nliber Jul 22, 2022 •

edited

dalg24 Jul 22, 2022

masterleinad Jul 25, 2022

dalg24 Jul 26, 2022

nliber Jul 22, 2022 •

edited

crtrott commented Jul 25, 2022

dalg24 Jul 26, 2022

dalg24 Jul 26, 2022

masterleinad Jul 26, 2022

crtrott Jul 26, 2022

crtrott Jul 26, 2022

masterleinad Jul 26, 2022

nliber Jul 26, 2022

nliber Jul 26, 2022

crtrott commented Jul 26, 2022

masterleinad left a comment

dalg24 commented Jul 26, 2022

	if (is_initialized()) {
	if (is_initialized() \|\| is_finalized()) {

		@@ -287,6 +393,7 @@ std::vector<ExecSpace> partition_space(ExecSpace space,

		// Specializations requires after core definitions

	// Specializations requires after core definitions
	// Specializations required after core definitions

Warn in ScopeGuard about illegal usage #5250

Warn in ScopeGuard about illegal usage #5250

Conversation

crtrott commented Jul 21, 2022

dalg24 Jul 21, 2022 • edited

Choose a reason for hiding this comment

dalg24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalg24 left a comment

Choose a reason for hiding this comment

masterleinad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nliber Jul 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nliber Jul 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nliber Jul 22, 2022 • edited

Choose a reason for hiding this comment

crtrott commented Jul 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crtrott commented Jul 26, 2022

masterleinad left a comment

Choose a reason for hiding this comment

dalg24 commented Jul 26, 2022

dalg24 Jul 21, 2022 •

edited

nliber Jul 22, 2022 •

edited

nliber Jul 22, 2022 •

edited

nliber Jul 22, 2022 •

edited