Sylvestre Ledru [:Sylvestre] Reporter
	Description • 7 years ago

Building with gcc 8 and --enable-warnings-as-errors
it fails on:

 In file included from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/security/manager/ssl/Unified_cpp_security_manager_ssl2.cpp:20:
 /root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp: In member function 'nsresult nsSSLIOLayerHelpers::Init()':
 /root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1696:53: error: cast between incompatible function types from 'int (*)()' to 'PRAvailableFN' {aka 'int (*)(PRFileDesc*)'} [-Werror=cast-function-type]
      nsSSLIOLayerMethods.available = (PRAvailableFN) PSMAvailable;
...                                                      ^~~~~~~~~~~~

	Sylvestre Ledru [:Sylvestre] Reporter
	Comment 1 • 7 years ago

David, seems that fixing it isn't easy because of the complex type.
Would you be ok with ignoring the warning on this file/declaration?
Thanks

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 2 • 7 years ago

I think the definition of PSMAvailable is just wrong - it should have a PRFileDesc * argument (same with PSMAvailable64, I imagine) (those two functions aren't implemented or called anyway).

	Sylvestre Ledru [:Sylvestre] Reporter
	Comment 3 • 7 years ago

I only showed the first warning. The list is pretty long.

here is the full list:
    Finished dev [optimized + debuginfo] target(s) in 0.0 secs
In file included from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/security/manager/ssl/Unified_cpp_security_manager_ssl2.cpp:20:
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp: In member function 'nsresult nsSSLIOLayerHelpers::Init()':
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1696:53: error: cast between incompatible function types from 'int (*)()' to 'PRAvailableFN' {aka 'int (*)(PRFileDesc*)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.available = (PRAvailableFN) PSMAvailable;
                                                     ^~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1697:57: error: cast between incompatible function types from 'int64_t (*)()' {aka 'long int (*)()'} to 'PRAvailable64FN' {aka 'long int (*)(PRFileDesc*)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.available64 = (PRAvailable64FN) PSMAvailable64;
                                                         ^~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1698:45: error: cast between incompatible function types from 'PRStatus (*)()' to 'PRFsyncFN' {aka 'PRStatus (*)(PRFileDesc*)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.fsync = (PRFsyncFN) _PSM_InvalidStatus;
                                             ^~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1699:43: error: cast between incompatible function types from 'int (*)()' to 'PRSeekFN' {aka 'int (*)(PRFileDesc*, int, PRSeekWhence)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.seek = (PRSeekFN) _PSM_InvalidInt;
                                           ^~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1700:47: error: cast between incompatible function types from 'int64_t (*)()' {aka 'long int (*)()'} to 'PRSeek64FN' {aka 'long int (*)(PRFileDesc*, long int, PRSeekWhence)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.seek64 = (PRSeek64FN) _PSM_InvalidInt64;
                                               ^~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1701:51: error: cast between incompatible function types from 'PRStatus (*)()' to 'PRFileInfoFN' {aka 'PRStatus (*)(PRFileDesc*, PRFileInfo*)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.fileInfo = (PRFileInfoFN) _PSM_InvalidStatus;
                                                   ^~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1702:55: error: cast between incompatible function types from 'PRStatus (*)()' to 'PRFileInfo64FN' {aka 'PRStatus (*)(PRFileDesc*, PRFileInfo64*)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.fileInfo64 = (PRFileInfo64FN) _PSM_InvalidStatus;
                                                       ^~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1703:47: error: cast between incompatible function types from 'int (*)()' to 'PRWritevFN' {aka 'int (*)(PRFileDesc*, const PRIOVec*, int, unsigned int)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.writev = (PRWritevFN) _PSM_InvalidInt;
                                               ^~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1704:47: error: cast between incompatible function types from 'PRFileDesc* (*)()' to 'PRAcceptFN' {aka 'PRFileDesc* (*)(PRFileDesc*, PRNetAddr*, unsigned int)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.accept = (PRAcceptFN) _PSM_InvalidDesc;
                                               ^~~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1705:47: error: cast between incompatible function types from 'PRStatus (*)()' to 'PRListenFN' {aka 'PRStatus (*)(PRFileDesc*, int)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.listen = (PRListenFN) _PSM_InvalidStatus;
                                               ^~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1706:51: error: cast between incompatible function types from 'PRStatus (*)()' to 'PRShutdownFN' {aka 'PRStatus (*)(PRFileDesc*, int)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.shutdown = (PRShutdownFN) _PSM_InvalidStatus;
                                                   ^~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1707:51: error: cast between incompatible function types from 'int (*)()' to 'PRRecvfromFN' {aka 'int (*)(PRFileDesc*, void*, int, int, PRNetAddr*, unsigned int)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.recvfrom = (PRRecvfromFN) _PSM_InvalidInt;
                                                   ^~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1708:47: error: cast between incompatible function types from 'int (*)()' to 'PRSendtoFN' {aka 'int (*)(PRFileDesc*, const void*, int, int, const PRNetAddr*, unsigned int)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.sendto = (PRSendtoFN) _PSM_InvalidInt;
                                               ^~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1709:55: error: cast between incompatible function types from 'int (*)()' to 'PRAcceptreadFN' {aka 'int (*)(PRFileDesc*, PRFileDesc**, PRNetAddr**, void*, int, unsigned int)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.acceptread = (PRAcceptreadFN) _PSM_InvalidInt;
                                                       ^~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1710:59: error: cast between incompatible function types from 'int (*)()' to 'PRTransmitfileFN' {aka 'int (*)(PRFileDesc*, PRFileDesc*, const void*, int, PRTransmitFileFlags, unsigned int)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.transmitfile = (PRTransmitfileFN) _PSM_InvalidInt;
                                                           ^~~~~~~~~~~~~~~
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1711:51: error: cast between incompatible function types from 'int (*)()' to 'PRSendfileFN' {aka 'int (*)(PRFileDesc*, PRSendFileData*, PRTransmitFileFlags, unsigned int)'} [-Werror=cast-function-type]
     nsSSLIOLayerMethods.sendfile = (PRSendfileFN) _PSM_InvalidInt;
                                                   ^~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

	Masatoshi Kimura [:emk]
	Comment 4 • 7 years ago

Same for all following warnings. Functions should have right parameters even though they are unused anyway.

	Sylvestre Ledru [:Sylvestre] Reporter
	Comment 5 • 7 years ago

So, I know how to fix it when we have only one argument
but with more complex, it is failing with
/root/firefox-gcc-last/security/manager/ssl/nsNSSIOLayer.cpp:1698:55: error: expected primary-expression before '*' token
     nsSSLIOLayerMethods.fsync = (PRFsyncFN)(PRFileDesc*, int, PRSeekWhence) _PSM_InvalidStatus;
                                                       ^

	Masatoshi Kimura [:emk]
	Comment 6 • 7 years ago

You should add parameters to the function declaration, not to the cast expression.

(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> So, I know how to fix it when we have only one argument

Actually you don't yet. `(PRAvailableFN)(PRFileDesc*)PSMAvailable;` is also wrong. Compiler could not warn this because it happens to have a valid syntax, but the semantics is bogus. You should change
  PSMAvailable(void)
to
  PSMAvailable(PRFileDesc *fd)
here:
https://dxr.mozilla.org/mozilla-central/rev/27fd083ed7ee5782e52a5eaf0286c5ffa8b35a9e/security/manager/ssl/nsNSSIOLayer.cpp#1598

Also, you can't share the same function if the parameter list is different. So you will have to add dummy functions instead of sharing _PSM_InvalidStatus. (Honestly, I'm surprised this is not broken until now.)

For example, you will have to add following functions and use them instead of _PSM_InvalidStatus.
static PRStatus _PSM_FsyncDummy(PRFileDesc *fd) { return _PSM_InvalidStatus(); }
static PRStatus _PSM_FileInfoDummy(PRFileDesc *fd, PRFileInfo *info) { return _PSM_InvalidStatus(); }
static PRStatus _PSM_FileInfo64Dummy(PRFileDesc *fd, PRFileInfo64 *info) { return _PSM_InvalidStatus(); }
static PRStatus _PSM_PRListenDummy(PRFileDesc *fd, PRIntn backlog) { return _PSM_InvalidStatus(); }
static PRStatus _PSM_PRShutdownDummy(PRFileDesc *fd, PRIntn how) { return _PSM_InvalidStatus(); }

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 7 • 7 years ago

The more I look at this code block, the more I think someone misunderstood how this was supposed to work. I'll find someone to address this, or I'll just do it myself.

	Sylvestre Ledru [:Sylvestre] Reporter
	Comment 8 • 7 years ago

Thanks David!

	Masatoshi Kimura [:emk]
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8956168 [details]
bug 1438196 - fix nsSSLIOLayerMethods definitions

https://reviewboard.mozilla.org/r/225082/#review231116

::: security/manager/ssl/nsNSSIOLayer.cpp:1662
(Diff revision 1)
>          "security.tls.insecure_fallback_hosts");
>    }
>  }
>  
> +#define UNIMPLEMENTED_PR_IO_METHOD(name, return_type, return_value, ...) \
> +static return_type _Unimplemented_##name(__VA_ARGS__)                    \

Please do not use identifiers beginning with an underscore followed by an uppercase character. It is always reserved to the implementation.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 11 • 7 years ago
mozreview-review-reply

Comment on attachment 8956168 [details]
bug 1438196 - fix nsSSLIOLayerMethods definitions

https://reviewboard.mozilla.org/r/225082/#review231116

> Please do not use identifiers beginning with an underscore followed by an uppercase character. It is always reserved to the implementation.

Huh - TIL.

	Franziskus Kiefer [:franziskus]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8956168 [details]
bug 1438196 - fix nsSSLIOLayerMethods definitions

https://reviewboard.mozilla.org/r/225082/#review231186

looks good!

::: security/manager/ssl/nsNSSIOLayer.cpp:1663
(Diff revision 2)
>    }
>  }
>  
> +#define UNIMPLEMENTED_PR_IO_METHOD(name, return_type, return_value, ...) \
> +static return_type                                                       \
> +Unimplemented_##name(__VA_ARGS__)                                        \

I really don't like macros in C++, they make error messages even less readable. But I guess it's fine here. We should never call these functions anyway.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 14 • 7 years ago
mozreview-review-reply

Comment on attachment 8956168 [details]
bug 1438196 - fix nsSSLIOLayerMethods definitions

https://reviewboard.mozilla.org/r/225082/#review231186

> I really don't like macros in C++, they make error messages even less readable. But I guess it's fine here. We should never call these functions anyway.

Yeah, I agree. I initially couldn't figure out how to do it with a variadic template, but I went back and figured it out - let me know what you think.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 16 • 7 years ago

ni? just so you see this (see comment 14)

	Franziskus Kiefer [:franziskus]
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8956168 [details]
bug 1438196 - fix nsSSLIOLayerMethods definitions

https://reviewboard.mozilla.org/r/225082/#review231534

Ah that's of corse way nicer :) Thanks! Ship it.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 19 • 7 years ago

Cool. Turns out available/available64 are reachable, so I had to change those back to how they were (they just set an error and return -1).
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d6db193d18f79eb02bc293bdbd074cdcfbdf6cb

	Pulsebot
	Comment 20 • 7 years ago

Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1683e068e94
fix nsSSLIOLayerMethods definitions r=fkiefer

	Cosmin Sabou [:CosminS]
	Comment 21 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e1683e068e94