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

Add Caffeine's own custom assert subroutine #76

Merged
merged 8 commits into from
Mar 27, 2024
Merged

Add Caffeine's own custom assert subroutine #76

merged 8 commits into from
Mar 27, 2024

Conversation

rouson
Copy link
Collaborator

@rouson rouson commented Mar 14, 2024

No description provided.

Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

This is a great improvement license-wise. I added a few minor requests for changes.

I'd also like to raise a bigger question, which is somewhat orthogonal to this PRs replacement:

This assertion library is currently implemented mostly in Fortran, but it relies upon the C preprocessor (at least when compiling the assertion module) for the critical control knob that enables or disables assertions. Given that dependency, what is the rationale for shunning additional dependence on the C preprocessor that would allow for a stronger assertion facility?

In particular, if we implement the assert() invocation itself using a C preprocessor macro, then we can easily deploy the following improvements:

  1. We can automatically augment the diagnostic message to include the source text for the expression that failed, and the file name and line number of the failed assertion (analogous to POSIX/C assert). This notably provides alot of valuable information about the failure with zero additional effort by the caller.
  2. We can trivially ensure that when assertions are disabled at compile time, the assertion invocations are strongly guaranteed to compile away entirely with zero runtime impact. The current implementation can only achieve that with strong compiler analysis and optimization, and might still fall short (notably whenever it's difficult to prove the argument expressions are side-effect-free).

To be clear, I'm not arguing to adopt heavy use of C preprocessor macros in Caffeine. However assertions are a specific software engineering feature that macros are very well-suited to implement in a powerful way, so what's the rationale to neglect that?

src/caffeine/caffeine_assert/caffeine_assert_s.f90 Outdated Show resolved Hide resolved
src/caffeine/caffeine_assert/caffeine_assert_m.F90 Outdated Show resolved Hide resolved
src/caffeine/caffeine_assert/caffeine_assert_s.f90 Outdated Show resolved Hide resolved
@bonachea
Copy link
Member

As a motivating example, below is example diagnostic output from an assertion failure provided by the UPC++ assertion facility, with GASNet's auto-backtrace support enabled.

The caller's invocation is simply the following line: (where the variable x actually has the value 3)

UPCXX_ASSERT(x > 10);

The resulting diagnostic failure output:

*** FATAL ERROR (proc 0): 
//////////////////////////////////////////////////////////////////////
UPC++ assertion failure:
 on process 0 (pcp-d-5)
 at assert2.cpp:5
 in function: void bar(int)

Failed condition: x > 10

To have UPC++ freeze during these errors so you can attach a debugger,
rerun the program with GASNET_FREEZE_ON_ERROR=1 in the environment.
//////////////////////////////////////////////////////////////////////

*** Details for bug reporting (proc 0): config=RELEASE=2023.9.0,SPEC=1.20,PTR=64bit,debug,SEQ,timers_native,membars_native,atomics_native,atomic32_native,atomic64_native compiler=GNU/13.2.0 sys=x86_64-pc-linux-gnu
[0] Invoking GDB for backtrace...
[0] /usr/local/pkg/gdb/newest/bin/gdb -nx -batch -x /tmp/gasnet_tebx6e '/home/pcp1/bonachea/UPC/code/a.out' 26879
[0] [Thread debugging using libthread_db enabled]
[0] Using host libthread_db library "/usr/lib64/libthread_db.so.1".
[0] 0x00007f81b75a0a3c in waitpid () from /usr/lib64/libc.so.6
[0] #0  0x00007f81b75a0a3c in waitpid () from /usr/lib64/libc.so.6
[0] #1  0x00007f81b751ede2 in do_system () from /usr/lib64/libc.so.6
[0] #2  0x00000000004f4a92 in gasneti_system_redirected (cmd=0xaa0de0 <cmd> "/usr/local/pkg/gdb/newest/bin/gdb -nx -batch -x /tmp/gasnet_tebx6e '/home/pcp1/bonachea/UPC/code/a.out' 26879", stdout_fd=3) at /tmp/upcxx-nightly-2024.03.13-dirac-gcc/bld/upcxx_install/berkeleylab-upcxx-develop/bld/GASNet-stable/gasnet_tools.c:1679
[0] #3  0x00000000004f5254 in gasneti_bt_gdb (fd=3) at /tmp/upcxx-nightly-2024.03.13-dirac-gcc/bld/upcxx_install/berkeleylab-upcxx-develop/bld/GASNet-stable/gasnet_tools.c:1944
[0] #4  0x00000000004f5a15 in gasneti_print_backtrace (fd=2) at /tmp/upcxx-nightly-2024.03.13-dirac-gcc/bld/upcxx_install/berkeleylab-upcxx-develop/bld/GASNet-stable/gasnet_tools.c:2228
[0] #5  0x00000000004f5fe9 in _gasneti_print_backtrace_ifenabled (fd=2) at /tmp/upcxx-nightly-2024.03.13-dirac-gcc/bld/upcxx_install/berkeleylab-upcxx-develop/bld/GASNet-stable/gasnet_tools.c:2359
[0] #6  0x00000000004f3415 in gasneti_error_abort () at /tmp/upcxx-nightly-2024.03.13-dirac-gcc/bld/upcxx_install/berkeleylab-upcxx-develop/bld/GASNet-stable/gasnet_tools.c:807
[0] #7  0x00000000004f3686 in gasneti_fatalerror_nopos (msg=0x7987cd "\n%s") at /tmp/upcxx-nightly-2024.03.13-dirac-gcc/bld/upcxx_install/berkeleylab-upcxx-develop/bld/GASNet-stable/gasnet_tools.c:851
[0] #8  0x00000000004734bb in upcxx::detail::fatal_error (msg=0xcb5530 "Failed condition: x > 10", title=0x7987d1 "assertion failure", func=0x77cfcc "void bar(int)", file=0x77cfc0 "assert2.cpp", line=5) at /tmp/upcxx-nightly-2024.03.13-dirac-gcc/bld/upcxx_install/berkeleylab-upcxx-develop/src/./diagnostic.cpp:63
[0] #9  0x00000000004734f1 in upcxx::detail::assert_failed (func=0x77cfcc "void bar(int)", file=0x77cfc0 "assert2.cpp", line=5, msg=0xcb5530 "Failed condition: x > 10") at /tmp/upcxx-nightly-2024.03.13-dirac-gcc/bld/upcxx_install/berkeleylab-upcxx-develop/src/./diagnostic.cpp:76
[0] #10 0x00000000004058aa in upcxx::detail::assert_failed (func=0x77cfcc "void bar(int)", file=0x77cfc0 "assert2.cpp", line=5, str=...) at /usr/local/pkg/upcxx-dirac/gcc-13.2.0/nightly-2024.03.13/include/upcxx/diagnostic.hpp:22
[0] #11 0x00000000004057cc in bar (x=3) at assert2.cpp:5
[0] #12 0x0000000000405844 in foo (x=3) at assert2.cpp:9
[0] #13 0x0000000000405865 in main () at assert2.cpp:16
[0] [Inferior 1 (process 26879) detached]
*** Caught a fatal signal (proc 0): SIGABRT(6)
*** Details for bug reporting (proc 0): config=RELEASE=2023.9.0,SPEC=1.20,PTR=64bit,debug,SEQ,timers_native,membars_native,atomics_native,atomic32_native,atomic64_native compiler=GNU/13.2.0 sys=x86_64-pc-linux-gnu
Abort

@rouson
Copy link
Collaborator Author

rouson commented Mar 15, 2024

@bonachea these are great suggestions and I'm supportive of the approach you're recommending. @ktras and I just discussed this PR and agreed that a live discussion will help with deciding a path forward so I'll respond in more detail during our next call. For now, here are a few things to consider:

  1. Strictly speaking, we're not guaranteed that every Fortran compiler invokes the C preprocessor. In the case of NAG, for example, the relevant flag is named -fpp for Fortran pre-preprocessor. I don't know what, if any differences there are between NAG's Fortran pre-processor and the C pre-processor.
  2. There is an upcoming Fortran preprocessor in the works so we might need to keep an eye on that for compatibility with any solution we choose.
  3. Will the proposed approach work on Windows? I ask because of your mention of POSIX. Maybe this is a moot point though because Caffeine uses GASNet-EX, which understandably doesn't support Windows. I'm sure the LLVM flang user community will eventually want support for Windows, but we could defer that to some other non-Caffeine solution that someone else implements.

@ktras ktras requested a review from bonachea March 21, 2024 19:34
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

New changes look good, I think we're almost there!

Added a few final minor requests.

src/caffeine/caffeine_assert_s.F90 Outdated Show resolved Hide resolved
src/caffeine/caffeine_assert_s.F90 Outdated Show resolved Hide resolved
@ktras ktras requested a review from bonachea March 26, 2024 17:59
@ktras ktras force-pushed the caffeine-assert branch from aeaa91e to 7404cd0 Compare March 27, 2024 16:16
@ktras ktras merged commit aaeda25 into main Mar 27, 2024
6 checks passed
@ktras ktras deleted the caffeine-assert branch March 27, 2024 16:22
@bonachea bonachea mentioned this pull request Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants