| Message ID | CAKdteOYC8t+K9iyDrTQw9PhXRTKSkEOM933W6c2rnFZwG8kHFA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add forward declarations | expand |
On 10/01/2018 05:37 PM, Christophe Lyon wrote: > Hi, > > While building newlib for Aarch64, I noticed several warnings because > of missing prototypes. I am not familiar enough to know why the same > warnings do not appear when building for Arm. > > This patch adds the missing prototypes, tested by rebuilding for > Aarch64 and Arm, it removed the warnings and didn't generate any > error. > > OK? > > Christophe A primary reason for prototypes is that they are for checking, and fully-proper checking is a single header file that checks both the source providing the function as well as places that call the function. That is, adding prototypes in the function definition file purely for the purposes of avoiding missing prototype warnings is missing the real usefulness behind the warning and giving a false sense of security. (You'd be better off just taking -Wmissing-prototypes out of your options to avoid those particular warnings.) The prototypes need to be added to a header file, not the functions' own source. Put another way, as a general goal I'd think we ought to be fixing the cause of a warning to remove the warning, rather than doing things to "hide" or "mask" the warning (which could be considered as kludges). Putting protoypes into the function source file is clearly masking the deficiency that there is not a header file with prototypes, rather than fixing the actual deficiency. While there certainly are times when addressing the root cause of a warning is not possible, this does not seem to be one where it is appropriate. Probably something like sys/_syscall.h (or some such name) should be created and used. We should be cleaning up bad practice when it exists, not furthering it. So, definitely a good thing to get rid of the warnings, but there is a much better approach to take in this case. Corrina or Jeff, any suggestions for a good place for syscall prototypes? (My first thought was sys/syscalls.h, but the Linux syscall(2) manpage mentions that related to syscall() and "For SYS_xxx definitions." Perhaps it would be OK, but perhaps not, too, which is why I ended up suggesting _syscall.h.) Does someone know about a precedent in BSD or the like? Craig
On Tue, 2 Oct 2018 at 03:04, Craig Howland <howland@lgsinnovations.com> wrote: > > On 10/01/2018 05:37 PM, Christophe Lyon wrote: > > Hi, > > > > While building newlib for Aarch64, I noticed several warnings because > > of missing prototypes. I am not familiar enough to know why the same > > warnings do not appear when building for Arm. > > > > This patch adds the missing prototypes, tested by rebuilding for > > Aarch64 and Arm, it removed the warnings and didn't generate any > > error. > > > > OK? > > > > Christophe > A primary reason for prototypes is that they are for checking, and fully-proper > checking is a single header file that checks both the source providing the > function as well as places that call the function. That is, adding prototypes > in the function definition file purely for the purposes of avoiding missing > prototype warnings is missing the real usefulness behind the warning and giving > a false sense of security. (You'd be better off just taking > -Wmissing-prototypes out of your options to avoid those particular warnings.) > The prototypes need to be added to a header file, not the functions' own source. > > Put another way, as a general goal I'd think we ought to be fixing the cause of > a warning to remove the warning, rather than doing things to "hide" or "mask" > the warning (which could be considered as kludges). Putting protoypes into the > function source file is clearly masking the deficiency that there is not a > header file with prototypes, rather than fixing the actual deficiency. While > there certainly are times when addressing the root cause of a warning is not > possible, this does not seem to be one where it is appropriate. Probably > something like sys/_syscall.h (or some such name) should be created and used. > We should be cleaning up bad practice when it exists, not furthering it. So, > definitely a good thing to get rid of the warnings, but there is a much better > approach to take in this case. > > Corrina or Jeff, any suggestions for a good place for syscall prototypes? (My > first thought was sys/syscalls.h, but the Linux syscall(2) manpage mentions that > related to syscall() and "For SYS_xxx definitions." Perhaps it would be OK, but > perhaps not, too, which is why I ended up suggesting _syscall.h.) Does someone > know about a precedent in BSD or the like? > I agree with you, one of my intentions was precisely to get knowledgeable feedback because this patch indeed looks a little bit like a kludge :) I've noticed that the prototypes are conditionally defined in newlib/libc/include/sys/unistd.h under #ifdef _COMPILING_NEWLIB but I'm not familiar enough with newlib build system to know if this is the file to include or not. In particular, as I said before, the warnings appear when building for Aarch64 and not for Arm, so different code paths are used at least by these two targets. > Craig
On Mon, Oct 1, 2018 at 9:04 PM, Craig Howland <howland@lgsinnovations.com> wrote: > On 10/01/2018 05:37 PM, Christophe Lyon wrote: > >> Hi, >> >> While building newlib for Aarch64, I noticed several warnings because >> of missing prototypes. I am not familiar enough to know why the same >> warnings do not appear when building for Arm. >> >> This patch adds the missing prototypes, tested by rebuilding for >> Aarch64 and Arm, it removed the warnings and didn't generate any >> error. >> >> OK? >> >> Christophe >> > A primary reason for prototypes is that they are for checking, and > fully-proper checking is a single header file that checks both the source > providing the function as well as places that call the function. That is, > adding prototypes in the function definition file purely for the purposes > of avoiding missing prototype warnings is missing the real usefulness > behind the warning and giving a false sense of security. (You'd be better > off just taking -Wmissing-prototypes out of your options to avoid those > particular warnings.) The prototypes need to be added to a header file, > not the functions' own source. > > Put another way, as a general goal I'd think we ought to be fixing the > cause of a warning to remove the warning, rather than doing things to > "hide" or "mask" the warning (which could be considered as kludges). > Putting protoypes into the function source file is clearly masking the > deficiency that there is not a header file with prototypes, rather than > fixing the actual deficiency. While there certainly are times when > addressing the root cause of a warning is not possible, this does not seem > to be one where it is appropriate. Probably something like sys/_syscall.h > (or some such name) should be created and used. We should be cleaning up > bad practice when it exists, not furthering it. So, definitely a good > thing to get rid of the warnings, but there is a much better approach to > take in this case. > > Corrina or Jeff, any suggestions for a good place for syscall prototypes? > (My first thought was sys/syscalls.h, but the Linux syscall(2) manpage > mentions that related to syscall() and "For SYS_xxx definitions." Perhaps > it would be OK, but perhaps not, too, which is why I ended up suggesting > _syscall.h.) Does someone know about a precedent in BSD or the like? > My suggestion is to set the _COMPILING_NEWLIB flag in configure.host for aarch64 as is already done for arm-*-*. -- Jeff J. > Craig >
On Tue, 2 Oct 2018 at 17:31, Jeff Johnston <jjohnstn@redhat.com> wrote: > > On Mon, Oct 1, 2018 at 9:04 PM, Craig Howland <howland@lgsinnovations.com> > wrote: > > > On 10/01/2018 05:37 PM, Christophe Lyon wrote: > > > >> Hi, > >> > >> While building newlib for Aarch64, I noticed several warnings because > >> of missing prototypes. I am not familiar enough to know why the same > >> warnings do not appear when building for Arm. > >> > >> This patch adds the missing prototypes, tested by rebuilding for > >> Aarch64 and Arm, it removed the warnings and didn't generate any > >> error. > >> > >> OK? > >> > >> Christophe > >> > > A primary reason for prototypes is that they are for checking, and > > fully-proper checking is a single header file that checks both the source > > providing the function as well as places that call the function. That is, > > adding prototypes in the function definition file purely for the purposes > > of avoiding missing prototype warnings is missing the real usefulness > > behind the warning and giving a false sense of security. (You'd be better > > off just taking -Wmissing-prototypes out of your options to avoid those > > particular warnings.) The prototypes need to be added to a header file, > > not the functions' own source. > > > > Put another way, as a general goal I'd think we ought to be fixing the > > cause of a warning to remove the warning, rather than doing things to > > "hide" or "mask" the warning (which could be considered as kludges). > > Putting protoypes into the function source file is clearly masking the > > deficiency that there is not a header file with prototypes, rather than > > fixing the actual deficiency. While there certainly are times when > > addressing the root cause of a warning is not possible, this does not seem > > to be one where it is appropriate. Probably something like sys/_syscall.h > > (or some such name) should be created and used. We should be cleaning up > > bad practice when it exists, not furthering it. So, definitely a good > > thing to get rid of the warnings, but there is a much better approach to > > take in this case. > > > > Corrina or Jeff, any suggestions for a good place for syscall prototypes? > > (My first thought was sys/syscalls.h, but the Linux syscall(2) manpage > > mentions that related to syscall() and "For SYS_xxx definitions." Perhaps > > it would be OK, but perhaps not, too, which is why I ended up suggesting > > _syscall.h.) Does someone know about a precedent in BSD or the like? > > > > My suggestion is to set the _COMPILING_NEWLIB flag in configure.host for > aarch64 as is already done for arm-*-*. > OK, thanks for the suggestion, it works. Is this new patch OK? > -- Jeff J. > > > > Craig > > commit dbfb9670991b3b3c31b98b92f388d4235fa7b9ca Author: Christophe Lyon <christophe.lyon@linaro.org> Date: Mon Oct 1 21:16:40 2018 +0000 Define _COMPILING_NEWLIB on aarch64 to define function prototypes from unistd.h. 2018-10-01 Christophe Lyon <christophe.lyon@linaro.org> * newlib/configure.host: Define _COMPILING_NEWLIB for aarch64. diff --git a/newlib/configure.host b/newlib/configure.host index c5b7734..9e809c9 100644 --- a/newlib/configure.host +++ b/newlib/configure.host @@ -438,6 +438,9 @@ case "${host}" in sys_dir=a29khif signal_dir= ;; + aarch64*-*-*) + newlib_cflags="${newlib_cflags} -D_COMPILING_NEWLIB" + ;; arm*-*-*) newlib_cflags="${newlib_cflags} -D_COMPILING_NEWLIB" sys_dir=arm
On 05/10/18 10:22, Christophe Lyon wrote: > On Tue, 2 Oct 2018 at 17:31, Jeff Johnston <jjohnstn@redhat.com> wrote: >> >> On Mon, Oct 1, 2018 at 9:04 PM, Craig Howland <howland@lgsinnovations.com> >> wrote: >> >>> On 10/01/2018 05:37 PM, Christophe Lyon wrote: >>> >>>> Hi, >>>> >>>> While building newlib for Aarch64, I noticed several warnings because >>>> of missing prototypes. I am not familiar enough to know why the same >>>> warnings do not appear when building for Arm. >>>> >>>> This patch adds the missing prototypes, tested by rebuilding for >>>> Aarch64 and Arm, it removed the warnings and didn't generate any >>>> error. >>>> >>>> OK? >>>> >>>> Christophe >>>> >>> A primary reason for prototypes is that they are for checking, and >>> fully-proper checking is a single header file that checks both the source >>> providing the function as well as places that call the function. That is, >>> adding prototypes in the function definition file purely for the purposes >>> of avoiding missing prototype warnings is missing the real usefulness >>> behind the warning and giving a false sense of security. (You'd be better >>> off just taking -Wmissing-prototypes out of your options to avoid those >>> particular warnings.) The prototypes need to be added to a header file, >>> not the functions' own source. >>> >>> Put another way, as a general goal I'd think we ought to be fixing the >>> cause of a warning to remove the warning, rather than doing things to >>> "hide" or "mask" the warning (which could be considered as kludges). >>> Putting protoypes into the function source file is clearly masking the >>> deficiency that there is not a header file with prototypes, rather than >>> fixing the actual deficiency. While there certainly are times when >>> addressing the root cause of a warning is not possible, this does not seem >>> to be one where it is appropriate. Probably something like sys/_syscall.h >>> (or some such name) should be created and used. We should be cleaning up >>> bad practice when it exists, not furthering it. So, definitely a good >>> thing to get rid of the warnings, but there is a much better approach to >>> take in this case. >>> >>> Corrina or Jeff, any suggestions for a good place for syscall prototypes? >>> (My first thought was sys/syscalls.h, but the Linux syscall(2) manpage >>> mentions that related to syscall() and "For SYS_xxx definitions." Perhaps >>> it would be OK, but perhaps not, too, which is why I ended up suggesting >>> _syscall.h.) Does someone know about a precedent in BSD or the like? >>> >> >> My suggestion is to set the _COMPILING_NEWLIB flag in configure.host for >> aarch64 as is already done for arm-*-*. >> > > OK, thanks for the suggestion, it works. Is this new patch OK? > Pushed. R. > >> -- Jeff J. >> >> >>> Craig >>> >>> >>> newlib-5.txt >>> >>> >>> commit dbfb9670991b3b3c31b98b92f388d4235fa7b9ca >>> Author: Christophe Lyon <christophe.lyon@linaro.org> >>> Date: Mon Oct 1 21:16:40 2018 +0000 >>> >>> Define _COMPILING_NEWLIB on aarch64 to define function prototypes from >>> unistd.h. >>> >>> 2018-10-01 Christophe Lyon <christophe.lyon@linaro.org> >>> >>> * newlib/configure.host: Define _COMPILING_NEWLIB for aarch64. >>> >>> diff --git a/newlib/configure.host b/newlib/configure.host >>> index c5b7734..9e809c9 100644 >>> --- a/newlib/configure.host >>> +++ b/newlib/configure.host >>> @@ -438,6 +438,9 @@ case "${host}" in >>> sys_dir=a29khif >>> signal_dir= >>> ;; >>> + aarch64*-*-*) >>> + newlib_cflags="${newlib_cflags} -D_COMPILING_NEWLIB" >>> + ;; >>> arm*-*-*) >>> newlib_cflags="${newlib_cflags} -D_COMPILING_NEWLIB" >>> sys_dir=arm
diff --git a/newlib/libc/reent/closer.c b/newlib/libc/reent/closer.c index deb34b0..1bef515 100644 --- a/newlib/libc/reent/closer.c +++ b/newlib/libc/reent/closer.c @@ -19,6 +19,9 @@ #undef errno extern int errno; +/* Forward declaration. */ +int _close (int); + /* FUNCTION <<_close_r>>---Reentrant version of close diff --git a/newlib/libc/reent/execr.c b/newlib/libc/reent/execr.c index 59b6122..90201b7 100644 --- a/newlib/libc/reent/execr.c +++ b/newlib/libc/reent/execr.c @@ -27,6 +27,10 @@ int _dummy_exec_syscalls = 1; #undef errno extern int errno; +/* Forward declarations. */ +int _execve(const char *filename, char *const argv[], char *const envp[]); +int _fork(); + /* FUNCTION <<_execve_r>>---Reentrant version of execve diff --git a/newlib/libc/reent/fcntlr.c b/newlib/libc/reent/fcntlr.c index cd19d22..27ab74c 100644 --- a/newlib/libc/reent/fcntlr.c +++ b/newlib/libc/reent/fcntlr.c @@ -21,6 +21,9 @@ #undef errno extern int errno; +/* Forward declaration. */ +int _fcntl (int, int, ...); + /* FUNCTION <<_fcntl_r>>---Reentrant version of fcntl diff --git a/newlib/libc/reent/fstatr.c b/newlib/libc/reent/fstatr.c index ec906c9..dcbaaee 100644 --- a/newlib/libc/reent/fstatr.c +++ b/newlib/libc/reent/fstatr.c @@ -25,6 +25,9 @@ int _dummy_fstat_syscalls = 1; #undef errno extern int errno; +/* Forward declaration. */ +int _fstat(int, struct stat *); + /* FUNCTION <<_fstat_r>>---Reentrant version of fstat diff --git a/newlib/libc/reent/gettimeofdayr.c b/newlib/libc/reent/gettimeofdayr.c index 9b982a9..361ee4f 100644 --- a/newlib/libc/reent/gettimeofdayr.c +++ b/newlib/libc/reent/gettimeofdayr.c @@ -28,6 +28,9 @@ int _dummy_gettimeofday_syscalls = 1; #undef errno extern int errno; +/* Forward declaration. */ +int _gettimeofday (struct timeval *, void *); + /* FUNCTION <<_gettimeofday_r>>---Reentrant version of gettimeofday diff --git a/newlib/libc/reent/isattyr.c b/newlib/libc/reent/isattyr.c index f21bf25..dcd609d 100644 --- a/newlib/libc/reent/isattyr.c +++ b/newlib/libc/reent/isattyr.c @@ -23,6 +23,9 @@ int _dummy_isatty_syscalls = 1; #undef errno extern int errno; +/* Forward declaration. */ +int _isatty (int); + /* FUNCTION <<_isatty_r>>---Reentrant version of isatty diff --git a/newlib/libc/reent/linkr.c b/newlib/libc/reent/linkr.c index b22da5f..701b8f3 100644 --- a/newlib/libc/reent/linkr.c +++ b/newlib/libc/reent/linkr.c @@ -24,6 +24,9 @@ int _dummy_link_syscalls = 1; #undef errno extern int errno; +/* Forward declaration. */ +int _link (const char *, const char *); + /* FUNCTION <<_link_r>>---Reentrant version of link diff --git a/newlib/libc/reent/lseekr.c b/newlib/libc/reent/lseekr.c index ac2daaa..d39f942 100644 --- a/newlib/libc/reent/lseekr.c +++ b/newlib/libc/reent/lseekr.c @@ -19,6 +19,9 @@ #undef errno extern int errno; +/* Forward declaration. */ +_off_t _lseek (int, _off_t, int); + /* FUNCTION <<_lseek_r>>---Reentrant version of lseek diff --git a/newlib/libc/reent/mkdirr.c b/newlib/libc/reent/mkdirr.c index fd72df6..f0bf749 100644 --- a/newlib/libc/reent/mkdirr.c +++ b/newlib/libc/reent/mkdirr.c @@ -19,6 +19,9 @@ #undef errno extern int errno; +/* Forward declaration. */ +int _mkdir (const char *, mode_t); + /* FUNCTION <<_mkdir_r>>---Reentrant version of mkdir diff --git a/newlib/libc/reent/openr.c b/newlib/libc/reent/openr.c index c6a7db5..a63de62 100644 --- a/newlib/libc/reent/openr.c +++ b/newlib/libc/reent/openr.c @@ -20,6 +20,9 @@ #undef errno extern int errno; +/* Forward declaration. */ +int _open (const char *, int, ...); + /* FUNCTION <<_open_r>>---Reentrant version of open diff --git a/newlib/libc/reent/readr.c b/newlib/libc/reent/readr.c index 7fccefd..09462ef 100644 --- a/newlib/libc/reent/readr.c +++ b/newlib/libc/reent/readr.c @@ -19,6 +19,9 @@ #undef errno extern int errno; +/* Forward declaration. */ +int _read (int, void *, size_t); + /* FUNCTION <<_read_r>>---Reentrant version of read diff --git a/newlib/libc/reent/signalr.c b/newlib/libc/reent/signalr.c index 345910e..217c368 100644 --- a/newlib/libc/reent/signalr.c +++ b/newlib/libc/reent/signalr.c @@ -25,6 +25,10 @@ int _dummy_link_syscalls = 1; #undef errno extern int errno; +/* Forward declarations. */ +int _kill (int, int); +int _getpid (); + /* FUNCTION <<_kill_r>>---Reentrant version of kill diff --git a/newlib/libc/reent/statr.c b/newlib/libc/reent/statr.c index 9388e02..f338940 100644 --- a/newlib/libc/reent/statr.c +++ b/newlib/libc/reent/statr.c @@ -26,6 +26,9 @@ int _dummy_stat_syscalls = 1; #undef errno extern int errno; +/* Forward declaration. */ +int _stat (const char *, struct stat *); + /* FUNCTION <<_stat_r>>---Reentrant version of stat diff --git a/newlib/libc/reent/timesr.c b/newlib/libc/reent/timesr.c index bb89003..ab80176 100644 --- a/newlib/libc/reent/timesr.c +++ b/newlib/libc/reent/timesr.c @@ -25,6 +25,9 @@ int _dummy_times_syscalls = 1; #undef errno extern int errno; +/* Forward declaration. */ +clock_t _times (struct tms *); + /* FUNCTION <<_times_r>>---Reentrant version of times diff --git a/newlib/libc/reent/unlinkr.c b/newlib/libc/reent/unlinkr.c index 41bac01..d6fd460 100644 --- a/newlib/libc/reent/unlinkr.c +++ b/newlib/libc/reent/unlinkr.c @@ -20,6 +20,9 @@ #undef errno extern int errno; +/* Forward declaration. */ +int _unlink (const char *); + /* FUNCTION <<_unlink_r>>---Reentrant version of unlink diff --git a/newlib/libc/reent/writer.c b/newlib/libc/reent/writer.c index 704aba1..ffb468d 100644 --- a/newlib/libc/reent/writer.c +++ b/newlib/libc/reent/writer.c @@ -19,6 +19,9 @@ #undef errno extern int errno; +/* Forward declaration. */ +int _write (int, const void *, size_t); + /* FUNCTION <<_write_r>>---Reentrant version of write diff --git a/newlib/libc/stdlib/strtorx.c b/newlib/libc/stdlib/strtorx.c index a35dabe..fcea9f2 100644 --- a/newlib/libc/stdlib/strtorx.c +++ b/newlib/libc/stdlib/strtorx.c @@ -55,6 +55,9 @@ THIS SOFTWARE. #define _4 0 #endif +/* Forward declaration. */ +long double nanl(const char *tagp); + void #ifdef KR_headers ULtox(L, bits, exp, k) __UShort *L; __ULong *bits; Long exp; int k; diff --git a/newlib/libc/syscalls/sysisatty.c b/newlib/libc/syscalls/sysisatty.c index 697c54b..2672ae1 100644 --- a/newlib/libc/syscalls/sysisatty.c +++ b/newlib/libc/syscalls/sysisatty.c @@ -3,6 +3,9 @@ #include <reent.h> #include <unistd.h> +/* Forward declaration. */ +int _isatty (int); + int isatty (int fd) {
Hi, While building newlib for Aarch64, I noticed several warnings because of missing prototypes. I am not familiar enough to know why the same warnings do not appear when building for Arm. This patch adds the missing prototypes, tested by rebuilding for Aarch64 and Arm, it removed the warnings and didn't generate any error. OK? Christophe commit 91b75af446b712f7f1a84d0afdf5d3f03a2377ed Author: Christophe Lyon <christophe.lyon@linaro.org> Date: Mon Oct 1 21:16:40 2018 +0000 Add forward declarations. 2018-10-01 Christophe Lyon <christophe.lyon@linaro.org> * newlib/libc/reent/closer.c: Add _close prototype. * newlib/libc/reent/execr.c: Add _execve and fork prototypes. * newlib/libc/reent/fcntlr.c: Add _fcntl prototype. * newlib/libc/reent/fstatr.c: Add _fstat prototype. * newlib/libc/reent/gettimeofdayr.c: Add _gettimeofday prototype. * newlib/libc/reent/isattyr.c: Add _isatty prototype. * newlib/libc/reent/linkr.c: Add _link prototype. * newlib/libc/reent/lseekr.c: Add _lseek prototype. * newlib/libc/reent/mkdirr.c: Add _mkdir prototype. * newlib/libc/reent/openr.c: Add _open prototype. * newlib/libc/reent/readr.c: Add _read prototype. * newlib/libc/reent/signalr.c: Add _kill and _getpid prototypes. * newlib/libc/reent/statr.c: Add _stat prototype. * newlib/libc/reent/timesr.c: Add _times prototype. * newlib/libc/reent/unlinkr.c: Add _unlink prototype. * newlib/libc/reent/writer.c: Add _write prototype. * newlib/libc/stdlib/strtorx: Add nanl prototype. * newlib/libc/syscalls/sysisatty.c: Add _isatty prototype.