Skip to content

Enable -Wstrict-prototypes #5888

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

Jump to bottom
Closed
wants to merge 14 commits into from
Closed

Enable -Wstrict-prototypes #5888

wants to merge 14 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jul 22, 2020

As I've been prompted by #5887

I'm not totally sure how to fix the GD and scanf ones.
pspell will need to have the warning disabled as we are working with system libraries.

Hopefully not too many more pop up from Azure.

nikic
nikic reviewed Jul 22, 2020
View reviewed changes
@Girgias Girgias force-pushed the strict-prototypes branch 13 times, most recently from 91ef875 to 4a088cb Compare July 30, 2020 00:28
@Girgias Girgias force-pushed the strict-prototypes branch 4 times, most recently from ed10223 to c2b8008 Compare July 30, 2020 03:27
@Girgias Girgias force-pushed the strict-prototypes branch 2 times, most recently from 13d8f50 to a014b7f Compare August 7, 2020 19:54
@twose
Copy link
Member

twose commented Sep 12, 2020

Ping

Warning on master:

/usr/local/include/php/Zend/zend_execute.h:314:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
 ZEND_API zend_string *get_active_function_or_method_name();

@kocsismate
Copy link
Member

kocsismate commented Sep 12, 2020 •
edited
Loading

Warning on master:

Ah no, I was the one introducing this one :/

@Girgias Girgias added this to the PHP 8.1 milestone Sep 24, 2020
@Girgias Girgias force-pushed the strict-prototypes branch 3 times, most recently from cca34d0 to 7a0ecd7 Compare October 16, 2020 17:23
@Girgias Girgias force-pushed the strict-prototypes branch 3 times, most recently from 814a15b to a7938ce Compare January 8, 2021 03:10
@krakjoe
Copy link
Member

krakjoe commented May 11, 2021

@Girgias what's the current status of this, milestone coming up, so if you intend to push on ... please push on ;)

@Girgias
Copy link
Member Author

Girgias commented May 11, 2021

I think I need some clarifications from @nikic again about this comment, as I got warnings if I didn't specify those.

nikic
nikic reviewed May 12, 2021
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

After looking a bit closer at how this works, I'm okay with the direction, including changes to .c files. Can you please directly commit the parts that just dumbly add (void) to signatures?

ext/mysqli/mysqli_warning.c Outdated
@@ -129,7 +129,7 @@ MYSQLI_WARNING * php_get_warnings(MYSQLND_CONN_DATA * mysql)

for (;;) {
zval *entry;
int errno;
int php_mysqli_errno;
Copy link
Member

Choose a reason for hiding this comment

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

Curious, why are these renames necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Or rather, I can see why it is necessary, the question is more why -Wstrict-prototypes specifically makes it necessary. Does it contain some special check for errno?

Regarding the change itself, I don't think we should have a php_ prefix for local variables. This should be just mysqli_errno, or else some variation on the name (err_num).

Copy link
Member

Choose a reason for hiding this comment

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

Can someone explain it to me also? Why are we renaming these?

Copy link
Member Author

Choose a reason for hiding this comment

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

errno is a global variable/macro defined by the implementation, as such redefining its prototype (here saying it is an int) generates a Wstrict-prototypes warning.
Moreover, overridding the value of errno might have unintended consequence as you are working with a global value (although very unlikely here).

I'll rename the usage here and in LDAP after I merged rebased on the void changes.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a bug then? Should it not be fixed separately on its own and in 7.4?

@Girgias Girgias force-pushed the strict-prototypes branch 2 times, most recently from 6706858 to 14e0574 Compare May 12, 2021 14:17
nikic
nikic reviewed May 12, 2021
View reviewed changes
@nikic
Copy link
Member

nikic commented May 12, 2021 

/Users/runner/work/1/s/sapi/fpm/fpm/events/kqueue.c:31:34: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
static int fpm_event_kqueue_clean();
                                 ^
                                  void

on macos

@Girgias Girgias force-pushed the strict-prototypes branch from 14e0574 to ee584d1 Compare May 12, 2021 14:53
nikic
nikic approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Please first land the remaining code changes and then the build-system changes.

ext/gd/config.m4
@@ -138,7 +138,8 @@ dnl
if test "$PHP_GD" != "no"; then

if test "$PHP_EXTERNAL_GD" = "no"; then
GD_CFLAGS=""
dnl Disable strict prototypes as GD takes advantages of variadic function signatures for function pointers.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dnl Disable strict prototypes as GD takes advantages of variadic function signatures for function pointers.
dnl Disable strict prototypes as GD takes advantage of variadic function signatures for function pointers.

ext/pcntl/pcntl.c
@@ -1385,6 +1386,11 @@ void pcntl_signal_dispatch()
sigprocmask(SIG_SETMASK, &old_mask, NULL);
}

static void pcntl_signal_dispatch_tick_function(int dummy_int, ZEND_ATTRIBUTE_UNUSED void *dummy_pointer)
Copy link
Member

Choose a reason for hiding this comment

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

Why is one of these marked as unused and the other isn't? As we disable this warning, I'd not add the attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because somehow I thought this function used the first param, clearly being drunk. Will remove the attribute.

@Girgias Girgias closed this in 96adc80 May 12, 2021
@Girgias Girgias deleted the strict-prototypes branch May 12, 2021 18:46
mvorisek
mvorisek reviewed May 13, 2021
View reviewed changes
ext/gd/config.m4
@@ -138,7 +138,8 @@ dnl
if test "$PHP_GD" != "no"; then

if test "$PHP_EXTERNAL_GD" = "no"; then
GD_CFLAGS=""
dnl Disable strict prototypes as GD takes advantages of variadic function signatures for function pointers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can gd be fixed somehow too?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because libgd does that and not the PHP implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvorisek mvorisek mvorisek left review comments

@kamil-tekiela kamil-tekiela kamil-tekiela left review comments

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
PHP 8.1
Development

Successfully merging this pull request may close these issues.
7 participants
@Girgias @twose @kocsismate @krakjoe @nikic @mvorisek @kamil-tekiela