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

Six mathoms need prototypes #15860

Open
p5pRT opened this issue Feb 4, 2017 · 14 comments
Open

Six mathoms need prototypes #15860

p5pRT opened this issue Feb 4, 2017 · 14 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented Feb 4, 2017

Migrated from rt.perl.org#130717 (status was 'open')

Searchable as RT130717$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 4, 2017

From @petdance

Created by @petdance

These six mathoms in mathoms.c need prototypes to satisfy clang's
-Wmissing-prototypes flag. They are not all as simple as adding
a line to embed.fnc, as some have been replaced by macros.

NV Perl_sv_2nv(pTHX_ SV *sv);

char *Perl_sv_2pv(pTHX_ SV *sv, STRLEN *lp);

NV Perl_huge(void);

I32 Perl_sv_eq(pTHX_ SV *sv1, SV *sv2);

char * Perl_sv_collxfrm(pTHX_ SV *const sv, STRLEN *const nxp);

bool Perl_sv_2bool(pTHX_ SV *const sv);

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.24.0:

Configured by andy at Sun Jun  5 23:28:46 CDT 2016.

Summary of my perl5 (revision 5 version 24 subversion 0) configuration:
   
  Platform:
    osname=linux, osvers=3.10.0-327.18.2.el7.x86_64, archname=x86_64-linux
    uname='linux clifford 3.10.0-327.18.2.el7.x86_64 #1 smp thu may 12 11:03:55 utc 2016 x86_64 x86_64 x86_64 gnulinux '
    config_args='-de -Dprefix=/home/andy/perl5/perlbrew/perls/perl-5.24.0 -Aeval:scriptdir=/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2',
    optimize='-O2',
    cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion='', gccversion='4.8.5 20150623 (Red Hat 4.8.5-4)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib /lib64 /usr/lib64 /usr/local/lib64
    libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.17.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.17'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'

Locally applied patches:
    Devel::PatchPerl 1.38


@INC for perl 5.24.0:
    /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0/x86_64-linux
    /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/site_perl/5.24.0
    /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0/x86_64-linux
    /home/andy/perl5/perlbrew/perls/perl-5.24.0/lib/5.24.0
    .


Environment for perl 5.24.0:
    HOME=/home/andy
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin:/home/andy/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin
    PERLBREW_BASHRC_VERSION=0.75
    PERLBREW_HOME=/home/andy/.perlbrew
    PERLBREW_MANPATH=/home/andy/perl5/perlbrew/perls/perl-5.24.0/man
    PERLBREW_PATH=/home/andy/perl5/perlbrew/bin:/home/andy/perl5/perlbrew/perls/perl-5.24.0/bin
    PERLBREW_PERL=perl-5.24.0
    PERLBREW_ROOT=/home/andy/perl5/perlbrew
    PERLBREW_VERSION=0.75
    PERLCRITIC=/home/andy/tw/Dev/perlcriticrc
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 4, 2017

From @jkeenan

On Sat, 04 Feb 2017 22​:02​:00 GMT, petdance wrote​:

This is a bug report for perl from andy@​petdance.com,
generated with the help of perlbug 1.40 running under perl 5.24.0.

-----------------------------------------------------------------
[Please describe your issue here]

These six mathoms in mathoms.c need prototypes to satisfy clang's
-Wmissing-prototypes flag. They are not all as simple as adding
a line to embed.fnc, as some have been replaced by macros.

NV Perl_sv_2nv(pTHX_ SV *sv);

char *Perl_sv_2pv(pTHX_ SV *sv, STRLEN *lp);

NV Perl_huge(void);

I32 Perl_sv_eq(pTHX_ SV *sv1, SV *sv2);

char * Perl_sv_collxfrm(pTHX_ SV *const sv, STRLEN *const nxp);

bool Perl_sv_2bool(pTHX_ SV *const sv);

Can you indicate how to configure and build Perl 5 blead so as to generate some of these warnings?

On Linux x86_64, I built blead twice, once unthreaded, once threaded.

#####
sh ./Configure -des -Dusedevel -Dcc=clang
sh ./Configure -des -Dusedevel -Dcc=clang -Dusethreads
#####

I then ran 'make test_prep' under 'script'. In each case, when I grepped for 'warning​:', I came up with nothing. (I know we've cleaned up a lot of compiler warnings over the past year. When I tested perl-5.24.1 built with clang, I got a fair amount of warnings; see http​://perl5.test-smoke.org/report/52884.)

So I don't yet see what needs to be corrected.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 4, 2017

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 5, 2017

From @petdance

Can you indicate how to configure and build Perl 5 blead so as to
generate some of these warnings?

I'm working on getting us to run clean under clang -Weverything. These six mathoms are part of what needs to be cleaned up to get that to happen.

Apply this patch and then configure with clang as your compiler.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 5, 2017

From @petdance

clang.patch
diff --git a/cflags.SH b/cflags.SH
index 3af1e97..7988b43 100755
--- a/cflags.SH
+++ b/cflags.SH
@@ -53,9 +53,33 @@ warn=''
 case "$gccversion" in
 '') ;;
 Intel*) ;; # The Intel C++ plays gcc on TV but is not really it.
+# clang -Weverything is what gcc -Wall (+ -Wextra) was supposed to be.
+*" Clang "*|*"Apple LLVM "*)
+    case "$ccflags" in
+    *-Weverything*) ;;
+    *)
+        # -Wno-unused-macros: We have tons of macros and it's OK if we don't use them.
+        # -Wno-gnu-statement-expression: Commonly used all over the codebase.
+        # -Wno-shadow: We have many common global variable names.  We would have to change many local variables.
+        # -Wno-overlength-strings: Compiler limit we don't care about
+        # -Wno-disabled-macro-expansion
+        # -Wno-variadic-macros: We use them.
+        # -Wno-cast-align
+        # -Wno-extended-offsetof
+        # -Wno-c11-extensions
+        # -Wno-format-nonliteral
+        # -Wno-switch-enum: This one is not mollified by a default in the case.  https://stackoverflow.com/questions/16631713/
+        # -Wno-conversion: We have far too many conversions to clean up
+        # -Wno-missing-variable-declarations: globals.c barfs
+        echo "cflags.SH: Adding -Weverything -Wno-unused-macros -Wno-gnu-statement-expression -Wno-shadow -Wno-overlength-strings -Wno-padded -Wno-sign-conversion -Wno-shorten-64-to-32 -Wno-disabled-macro-expansion -Wno-variadic-macros -Wno-cast-align -Wno-extended-offsetof -Wno-c11-extensions -Wno-format-nonliteral -Wno-switch-enum -Wno-conversion -Wno-missing-variable-declarations"
+        warn="$warn -Weverything -Wno-unused-macros -Wno-gnu-statement-expression -Wno-shadow -Wno-overlength-strings -Wno-padded -Wno-sign-conversion -Wno-shorten-64-to-32 -Wno-disabled-macro-expansion -Wno-variadic-macros -Wno-cast-align -Wno-extended-offsetof -Wno-c11-extensions -Wno-format-nonliteral -Wno-switch-enum -Wno-conversion -Wno-missing-variable-declarations" ;;
+    esac
+    ;;
 *)  case "$ccflags" in
     *-Wall*) ;;
-    *) warn="$warn -Wall" ;;
+    *)
+        echo "cflags.SH: Adding -Wall."
+        warn="$warn -Wall" ;;
     esac
     ;;
 esac
@@ -247,6 +271,17 @@ Intel*) ;; # # Is that you, Intel C++?
                      ;;
                   esac
                   ;;
+               -Wextra)
+                  # -Wextra is for (pure) gcc, not needed
+                  # with clang -Weverything.
+                  case " $warn " in
+                  *-Weverything*) ;;
+                  *)
+                     echo "cflags.SH: Adding $opt."
+                     warn="$warn $opt"
+                     ;;
+                  esac
+                  ;;
                *)
                   echo "cflags.SH: Adding $opt."
                   warn="$warn $opt"
@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 5, 2017

From @jkeenan

On Sun, 05 Feb 2017 02​:55​:23 GMT, petdance wrote​:

Can you indicate how to configure and build Perl 5 blead so as to
generate some of these warnings?

I'm working on getting us to run clean under clang -Weverything.
These six mathoms are part of what needs to be cleaned up to get that
to happen.

Apply this patch and then configure with clang as your compiler.

Yup, I see what you mean!

Anyone else who wants to see a lot of warnings, checkout out this branch​:

smoke-me/jkeenan/petdance/130717-Weverything

Thank you very much.
Jim Keenan

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 5, 2017

From @petdance

I've done a lot of work on my branch to quiet those, but I'm not submitting at this time. Right now, all I'm doing is nothing that these six functions need prototypes.

I may yet fix it myself. I'm marking it here for posterity.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 5, 2017

From @jkeenan

On Sun, 05 Feb 2017 03​:24​:14 GMT, jkeenan wrote​:

On Sun, 05 Feb 2017 02​:55​:23 GMT, petdance wrote​:

I'm working on getting us to run clean under clang -Weverything.
These six mathoms are part of what needs to be cleaned up to get that
to happen.

Apply this patch and then configure with clang as your compiler.

Yup, I see what you mean!

Anyone else who wants to see a lot of warnings, checkout out this branch​:

smoke-me/jkeenan/petdance/130717-Weverything

A smoke-test of blead on freebsd-11 -- where clang is the default compiler -- with Andy's patch for -Weverything​:

http​://perl5.test-smoke.org/report/53742

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 6, 2017

From @petdance

A smoke-test of blead on freebsd-11 -- where clang is the default
compiler -- with Andy's patch for -Weverything​:

I didn't give you my entire patch. I just told how I did my configuration so that -Weverything is set up. I'm not sure why you set up a smoke test on this.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 6, 2017

From @demerphq

On 6 February 2017 at 03​:09, Andy Lester via RT
<perlbug-followup@​perl.org> wrote​:

A smoke-test of blead on freebsd-11 -- where clang is the default
compiler -- with Andy's patch for -Weverything​:

I didn't give you my entire patch. I just told how I did my configuration so that -Weverything is set up. I'm not sure why you set up a smoke test on this.

Standard practice I should think. What you see might not be what everyone sees.

Does it do any harm? I'm sure James was just trying to be helpful.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 6, 2017

From @petdance

I didn't give you my entire patch. I just told how I did my configuration so that -Weverything is set up. I'm not sure why you set up a smoke test on this.

Does it do any harm? I'm sure James was just trying to be helpful.

I wasn't criticizing. There was no subtext.

I just don't understand why he set it up to smoke.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 6, 2017

From @demerphq

On 6 February 2017 at 05​:28, Andy Lester via RT
<perlbug-followup@​perl.org> wrote​:

I didn't give you my entire patch. I just told how I did my configuration so that -Weverything is set up. I'm not sure why you set up a smoke test on this.

Does it do any harm? I'm sure James was just trying to be helpful.

I wasn't criticizing. There was no subtext.

I just don't understand why he set it up to smoke.

I know more than once I've pushed a branch as a smoke-me when I only
really wanted to share a changeset through the master repo. Combo of
"why not" and finger memory....

cheers,
Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 6, 2017

From @jkeenan

On Mon, 06 Feb 2017 02​:09​:56 GMT, petdance wrote​:

A smoke-test of blead on freebsd-11 -- where clang is the default
compiler -- with Andy's patch for -Weverything​:

I didn't give you my entire patch. I just told how I did my
configuration so that -Weverything is set up. I'm not sure why you
set up a smoke test on this.

I did that only so that if anyone else wanted to see the impact of clang + -Weverything on the number of warnings thrown by 'make', they could do so.

At the present time we have next to no automated smoke testing of branches other than blead going on. (Dan Collins has two setups, neither of which use clang.) So other than the smoke tests which I manually kick off, your branch will hardly get smoke tested at all. I'll rename it by removing the smoke test part.

Thank you very much.
Jim Keenan
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 6, 2017

From @petdance

I did that only so that if anyone else wanted to see the impact of
clang + -Weverything on the number of warnings thrown by 'make', they
could do so.

That's cool. It's just that that's only part of what I've done so far. I've reduced the amount of noise, but I didn't post those changes in this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant