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

ARKODE error messages are being cutoff #461

Closed
Steven-Roberts opened this issue Apr 25, 2024 · 10 comments
Closed

ARKODE error messages are being cutoff #461

Steven-Roberts opened this issue Apr 25, 2024 · 10 comments
Labels

Comments

@Steven-Roberts
Copy link
Collaborator

Steven-Roberts commented Apr 25, 2024

I noticed that some ARKODE error messages are getting cut off, .e.g, At t = 0.001, mxstep steps taken before reaching tou. When SUNDIALS is built in double precision, some format specifiers seem to be for long double, which I think is the issue:

#elif defined(SUNDIALS_DOUBLE_PRECISION)
#define MSG_TIME "t = %lg"
#define MSG_TIME_H "t = %lg and h = %lg"
#define MSG_TIME_INT "t = %lg is not between tcur - hold = %lg and tcur = %lg."
#define MSG_TIME_TOUT "tout = %lg"
#define MSG_TIME_TSTOP "tstop = %lg"

Looks like the other _impl.h files have the same thing.

@Steven-Roberts Steven-Roberts changed the title Incorrect Format Specifies for Error Messages using Double Precision Incorrect Format Specifiers for Error Messages using Double Precision Apr 25, 2024
@balos1
Copy link
Member

balos1 commented Apr 25, 2024

Lower case l is not for long double, that would be upper case L. l actually has no effect in this case.

@balos1
Copy link
Member

balos1 commented Apr 25, 2024

I just tried to reproduce the cutoff message and was unable to:

[ERROR][rank 0][/Volumes/Workspace/SUNDIALS/repos/sundials/src/arkode/arkode.c:852][arkEvolve] At t = 0.061625, mxstep steps taken before reaching tout.

@balos1
Copy link
Member

balos1 commented Apr 25, 2024

Is it possible that your terminal is cutting off the output? Maybe try setting the environment variable SUNLOGGER_ERROR_FILENAME to a file.

@Steven-Roberts
Copy link
Collaborator Author

Whoops, looks like it isn't the format specifier then. I saw it could cause undefined behavior, but apparently that was only before C99. After a closer look, the message length here is somehow wrong when I step through in the debugger

size_t msglen = vsnprintf(NULL, 0, msgfmt, ap) + 1;

msgfmt has a length of 52, but msgfmt gets set to 51.

I'm seeing this with gcc 8.5 and 13.1. I might need to boil things down to a minimal example to make sure it's not a secondary effect.

@Steven-Roberts
Copy link
Collaborator Author

It could be that ap is used twice without va_copy or va_end: https://stackoverflow.com/q/55274350

@balos1
Copy link
Member

balos1 commented Apr 25, 2024

Can you try this?

diff --git a/src/arkode/arkode.c b/src/arkode/arkode.c
index eac2af422..5cdff8575 100644
--- a/src/arkode/arkode.c
+++ b/src/arkode/arkode.c
@@ -3328,12 +3328,17 @@ void arkProcessError(ARKodeMem ark_mem, int error_code, int line,
   /* Initialize the argument pointer variable
      (msgfmt is the last required argument to arkProcessError) */
   va_list ap;
-  va_start(ap, msgfmt);
 
   /* Compose the message */
+  va_start(ap, msgfmt);
   size_t msglen = vsnprintf(NULL, 0, msgfmt, ap) + 1;
+  va_end(ap);
+
   char* msg     = (char*)malloc(msglen);
+
+  va_start(ap, msgfmt);
   vsnprintf(msg, msglen, msgfmt, ap);
+  va_end(ap);
 
   do {
     if (ark_mem == NULL)
@@ -3362,7 +3367,6 @@ void arkProcessError(ARKodeMem ark_mem, int error_code, int line,
   while (0);
 
   /* Finalize argument processing */
-  va_end(ap);
   free(msg);
 
   return;

@Steven-Roberts
Copy link
Collaborator Author

Yep, that fixed it. Turns out msglen is fine now that it's printing out the correct time. I think you can make a PR from that exact diff to close this issue. Thanks!

@balos1
Copy link
Member

balos1 commented Apr 25, 2024

Cool. Ill note that this seems to be specific to Linux as it works fine with gcc 13 on my m1 mac.

@balos1
Copy link
Member

balos1 commented Apr 25, 2024

This is probably needed in all of the packages.

@balos1 balos1 changed the title Incorrect Format Specifiers for Error Messages using Double Precision ARKODE error messages are being cutoff Apr 25, 2024
balos1 added a commit that referenced this issue Apr 25, 2024
gardner48 added a commit that referenced this issue Apr 26, 2024
Fixed a bug that caused error messages to be cut off in some cases. Fixes #461

---------

Co-authored-by: David Gardner <gardner48@llnl.gov>
@balos1
Copy link
Member

balos1 commented Apr 26, 2024

Closed by #462

@balos1 balos1 closed this as completed Apr 26, 2024
gardner48 added a commit that referenced this issue Jun 20, 2024
Fixed a bug that caused error messages to be cut off in some cases. Fixes #461

---------

Co-authored-by: David Gardner <gardner48@llnl.gov>
balos1 added a commit that referenced this issue Sep 3, 2024
The `test_sundials_errors` test was failing for me which I traced down
to the handling of variable arguments. It's the same issue discussed in
#461 which was partially fixed in
#462. This PR should finish it off.

---------

Co-authored-by: Cody Balos <balos1@llnl.gov>
Co-authored-by: David Gardner <gardner48@llnl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants