Bug 1850198 - Changed api for fail_if and fail_unless
Summary: Changed api for fail_if and fail_unless
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: check
Version: 33
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jerry James
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-06-23 17:30 UTC by Lukas Slebodnik
Modified: 2021-03-05 11:42 UTC (History)
16 users (show)

Fixed In Version: check-0.15.2-1.fc33
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-03-05 11:42:26 UTC
Type: Bug


Attachments (Terms of Use)

Description Lukas Slebodnik 2020-06-23 17:30:42 UTC
Description of problem:
New version of check (indirectly) added required parameter `msg` to `fail_if` macro. Even though it is not clear from macro definition.

sh$ grep fail_if /usr/include/check.h 
#define fail_if(expr, ...)\

I read it as `expr` is required and we might have some optional parameters if needed. But simple code fails without msg

Version-Release number of selected component (if applicable):
check-0.15.0-1.fc33.x86_64

How reproducible:
Deterministic 

Steps to Reproduce:
1. dnf install -y gcc check-devel
2. cat > test.c <<EOF
#include <check.h>

int main(void) {
    fail_if(1);
}
EOF

3. gcc -Wall -Wextra test.c


Actual results:
In file included from test.c:1:
test.c: In function ‘main’:
test.c:5:5: error: too few arguments to function ‘_ck_assert_failed’
    5 |     fail_if(1);
      |     ^~~~~~~
/usr/include/check.h:502:27: note: declared here
  502 | CK_DLL_EXP void CK_EXPORT _ck_assert_failed(const char *file, int line,
      |                           ^~~~~~~~~~~~~~~~~

Expected results:
no failures.

Additional info:

Comment 2 Jerry James 2020-06-23 21:54:19 UTC
Sorry about that.  Upstream tried to add format arg checking but did it incorrectly.  That was my attempt at fixing the issue.  I've dropped the patch for now, but that means you get the upstream bug back.  Expect lots and lots of warning messages from the format arg checker.

I'll try to work out a way to fix the upstream bug without breaking fail_if.

Comment 3 Lukas Slebodnik 2020-06-24 09:06:35 UTC
(In reply to Jerry James from comment #2)
> Sorry about that.  Upstream tried to add format arg checking but did it
> incorrectly.  That was my attempt at fixing the issue.  I've dropped the
> patch for now, but that means you get the upstream bug back.  Expect lots
> and lots of warning messages from the format arg checker.
> 
> I'll try to work out a way to fix the upstream bug without breaking fail_if.

I am not against proper fix but it should be firstly fixed in the upstream.

Comment 4 Lukas Slebodnik 2020-07-24 08:22:35 UTC
Still reproducible even with newer version in rawhide

[build@aec39e3ac883 ~]$ rpm -q check-devel
check-devel-0.15.1-1.fc33.x86_64

[build@aec39e3ac883 ~]$ gcc -Wall -Wextra test.c
In file included from test.c:1:
test.c: In function ‘main’:
test.c:4:5: error: too few arguments to function ‘_ck_assert_failed’
    4 |     fail_if(1);
      |     ^~~~~~~
/usr/include/check.h:502:27: note: declared here
  502 | CK_DLL_EXP void CK_EXPORT _ck_assert_failed(const char *file, int line,
      |

Comment 5 Pavel Březina 2020-07-27 10:43:18 UTC
This breaks SSSD tests. Is there any ETA on the fix?

Comment 6 Pavel Březina 2020-07-28 13:47:10 UTC
Lukas submitted patches to make SSSD compatible with the changes, so it no longer blocks us.

Comment 7 Christine Caulfield 2020-07-29 10:05:37 UTC
I have this bug with libqb too.

Comment 8 Paul Howarth 2020-07-29 10:19:56 UTC
Affects proftpd too. I have a patch for proftpd that I'll be sending upstream later today.

I see that the fail_* macros are shown as "deprecated" in the check documentation anyway.

Comment 9 Jerry James 2020-08-03 19:42:57 UTC
I have applied upstream's fix and rebuilt in Rawhide.  Please try your builds again.

Also, it is true that the fail* macros are deprecated.  The ck_assert* macros should be used instead.

Comment 10 Lukas Slebodnik 2020-08-04 07:52:31 UTC
(In reply to Jerry James from comment #9)
> I have applied upstream's fix and rebuilt in Rawhide.  Please try your
> builds again.
> 

It allows missing message but does not allow message at all.
even though macro still allow them in the definition
```
#define fail_if(expr, ...)\
```

sh$ cat >test.c <<EOF
#include <check.h>


int main(void) {
    fail_if(1);
    fail_if(1, "message");
    fail_if(1, "message with format string: %d", 42);
    return 0;
}
EOF

sh$ gcc -Wall -Wextra -Werror test.c -lcheck
In file included from test.c:1:
test.c: In function ‘main’:
test.c:6:16: error: too many arguments for format [-Werror=format-extra-args]
    6 |     fail_if(1, "message");
      |                ^~~~~~~~~
test.c:7:16: error: too many arguments for format [-Werror=format-extra-args]
    7 |     fail_if(1, "message with format string: %d", 42);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Comment 11 Lukas Slebodnik 2020-08-04 07:53:23 UTC
It is more obvious after compiling source file after pre-processing

sh$ gcc -E -P -Wall -Wextra -Werror test.c -o test2.c
sh$ gcc -Wall -Wextra -Werror test2.c -lcheck
test2.c: In function ‘main’:
test2.c:847:71: error: too many arguments for format [-Werror=format-extra-args]
  847 |     (1) ? _ck_assert_failed("test.c", 6, "Failure '""1""' occurred" , "message", ((void *)0)) : _mark_point("test.c", 6);
      |                                                                       ^~~~~~~~~
test2.c:848:71: error: too many arguments for format [-Werror=format-extra-args]
  848 |     (1) ? _ck_assert_failed("test.c", 7, "Failure '""1""' occurred" , "message with format string: %d", 42, ((void *)0)) : _mark_point("test.c", 7);
      |                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Comment 12 Lukas Slebodnik 2020-08-04 08:10:37 UTC
Looks like there is not an easy way for all possible options
which could be used without any problem with check <= 0.15.0

```
    fail_if(1);
    fail_if(1, "message");
    fail_if(1, "message with format string: %d", 42);
```

IMHO there is a possible option.

Add required *msg* parameter to fail_if (yes backward incompatible change.
but it will allow to use format string checking which is very useful

#define fail_if(expr, msg ...)\

And allow fallback to old behavior if some macro is defined before check.h
that would require either new copy od function _ck_assert_failed which does not require
format string checking or disable it for _ck_assert_failed with ifdef

Comment 13 Lukas Slebodnik 2020-08-04 08:15:08 UTC
(In reply to Paul Howarth from comment #8)
> I see that the fail_* macros are shown as "deprecated" in the check
> documentation anyway.

Sure they are deprecated but still widely used and useful in some situation.

and IMHO it would be betler to drop deprecated macros rather than change behavior.

Comment 14 Jerry James 2020-08-04 15:24:58 UTC
(In reply to Lukas Slebodnik from comment #10)
> It allows missing message but does not allow message at all.
> even though macro still allow them in the definition

Sure it does.  The compiler issues a warning, but the warning isn't fatal unless ...

> sh$ gcc -Wall -Wextra -Werror test.c -lcheck

... you add -Werror to your build flags.  Try using the ck_assert* macros instead of the deprecated fail* macros if you want a warning-free build.

Comment 15 Jerry James 2020-08-04 15:26:53 UTC
(In reply to Lukas Slebodnik from comment #12)
> Looks like there is not an easy way for all possible options
> which could be used without any problem with check <= 0.15.0
> 
> ```
>     fail_if(1);
>     fail_if(1, "message");
>     fail_if(1, "message with format string: %d", 42);
> ```
> 
> IMHO there is a possible option.
> 
> Add required *msg* parameter to fail_if (yes backward incompatible change.
> but it will allow to use format string checking which is very useful
> 
> #define fail_if(expr, msg ...)\
> 
> And allow fallback to old behavior if some macro is defined before check.h
> that would require either new copy od function _ck_assert_failed which does
> not require
> format string checking or disable it for _ck_assert_failed with ifdef

Do you have a patch to do this?  If so, let's show it to upstream and see what they think of it.

Comment 16 Lukas Slebodnik 2020-08-08 20:47:46 UTC
(In reply to Jerry James from comment #14)
> (In reply to Lukas Slebodnik from comment #10)
> > It allows missing message but does not allow message at all.
> > even though macro still allow them in the definition
> 
> Sure it does.  The compiler issues a warning, but the warning isn't fatal
> unless ...
> 
> > sh$ gcc -Wall -Wextra -Werror test.c -lcheck
> 
> ... you add -Werror to your build flags.

It emits "-Wformat-extra-args". Werror is intentional for preventing unnecessary warnings.
Otherwise  you needn't notice them; especially if you rely on CI ehn compiling on many platforms. 

Sure we could Wformat-extra-args but that would be disabled for everything and not just for unit tests.
IIUC, the goal of libcheck was to  addt format string checking but it does not work for following cases.
(and they are most usefull cases)
    fail_if(1, "message");
    fail_if(1, "message with format string: %d", 42);

Using specialized ck_asser__* macros do not have varargs and thus do not benefit a lot from format string checking.


>  Try using the ck_assert* macros
> instead of the deprecated fail* macros if you want a warning-free build.

sh$ git grep -E "fail_(if|unless)" | wc -l
1948

We can image a better way of spending time rather then changing all usage in older tests.

Comment 17 Ben Cotton 2020-08-11 13:40:15 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 33 development cycle.
Changing version to 33.

Comment 18 Erik Ekman 2020-08-12 21:09:46 UTC
Check release 0.15.2 is supposed to fix this: https://github.com/libcheck/check/releases/tag/0.15.2


Note You need to log in before you can comment on or make changes to this bug.