Fix memory leaks in feature class

[leonerd]

We forgot to free many of the additional structures that were allocated as part of the new class feature. Here they are fixed.

[leonerd]

I've pushed a smoke-me branch of this as well
http://perl.develop-help.com/?b=smoke-me%2Fleonerd%2Ffeature-class-fix-leaks

[jkeenan]

One test failure being observed on Linux when using clang and building with ASAN and threads, as described in #20810.

$ uname -mrs; clang --version
Linux 5.10.0-18-amd64 x86_64
Debian clang version 11.0.1-2
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

$ env | grep DESTRUCT
PERL_DESTRUCT_LEVEL=2

$ gitcurr; ./perl -Ilib -v | head -2 | tail -1
feature-class-fix-leaks
This is perl 5, version 37, subversion 9 (v5.37.9 (v5.37.8-175-g348a97f8a1)) built for x86_64-linux-thread-multi

$ ./perl -Ilib -V:config_args
config_args='-des -Dusedevel -Dcc=clang -Accflags=-g -fno-omit-frame-pointer -fsanitize=address  -fno-common -fsanitize-blacklist=/home/jkeenan/gitwork/perl/asan_ignore -Aldflags=-fsanitize=address -Duseithreads';

$ cd t; TEST_JOBS=1 ./perl harness lib/croak.t re/pat.t re/pat_psycho_thr.t re/pat_psycho.t; cd -
lib/croak.t .......... 12/215 FILE: lib/croak/class ; line 101
PROG: 
use strict;
no warnings 'experimental::class';
use feature 'class';
class XXX {
  field $x = last;
}
EXPECTED:
Can't "last" out of field initialiser expression at - line 5.
EXIT STATUS: != 0
GOT:
Can't "last" out of field initialiser expression at - line 5.

=================================================================
==1900342==ERROR: LeakSanitizer: detected memory leaks

Indirect leak of 552 byte(s) in 1 object(s) allocated from:
    #0 0x4aecad in malloc (/home/jkeenan/gitwork/perl/perl+0x4aecad)
    #1 0x4deea8 in S_new_slab /home/jkeenan/gitwork/perl/op.c:254:22
    #2 0x4deea8 in Perl_Slab_Alloc /home/jkeenan/gitwork/perl/op.c:346:32
    #3 0x4fac25 in Perl_newOP /home/jkeenan/gitwork/perl/op.c:5614:5
    #4 0x62292e in Perl_yyparse /home/jkeenan/gitwork/perl/perly.y:1457:23
    #5 0x5517c7 in S_parse_body /home/jkeenan/gitwork/perl/perl.c:2614:9
    #6 0x5517c7 in perl_parse /home/jkeenan/gitwork/perl/perl.c:1923:9
    #7 0x4dea99 in main /home/jkeenan/gitwork/perl/perlmain.c:106:10
    #8 0x7f5718f8fd09 in __libc_start_main csu/../csu/libc-start.c:308:16

Indirect leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x4aee22 in calloc (/home/jkeenan/gitwork/perl/perl+0x4aee22)
    #1 0x4df691 in S_link_freed_op /home/jkeenan/gitwork/perl/op.c:284:36
    #2 0x4e04ff in Perl_Slab_Free /home/jkeenan/gitwork/perl/op.c:507:5
    #3 0x4e04ff in Perl_op_free /home/jkeenan/gitwork/perl/op.c:991:9
    #4 0x629d17 in Perl_cv_undef_flags /home/jkeenan/gitwork/perl/pad.c:337:13
    #5 0x81ebbb in Perl_sv_clear /home/jkeenan/gitwork/perl/sv.c:6717:13
    #6 0x8224c3 in Perl_sv_free2 /home/jkeenan/gitwork/perl/sv.c:7230:9
    #7 0x7c340d in Perl_SvREFCNT_dec /home/jkeenan/gitwork/perl/./sv_inline.h:689:13
    #8 0x7c340d in Perl_hv_undef_flags /home/jkeenan/gitwork/perl/hv.c:2328:11
    #9 0x8205d6 in Perl_sv_clear /home/jkeenan/gitwork/perl/sv.c:7006:21
    #10 0x8224c3 in Perl_sv_free2 /home/jkeenan/gitwork/perl/sv.c:7230:9
    #11 0x7fbcb7 in Perl_SvREFCNT_dec_NN /home/jkeenan/gitwork/perl/./sv_inline.h:703:9
    #12 0x7fbcb7 in do_clean_all /home/jkeenan/gitwork/perl/sv.c:574:5
    #13 0x7fbcb7 in S_visit /home/jkeenan/gitwork/perl/sv.c:391:17
    #14 0x7fbcb7 in Perl_sv_clean_all /home/jkeenan/gitwork/perl/sv.c:592:15
    #15 0x548657 in perl_destruct /home/jkeenan/gitwork/perl/perl.c:1333:12
    #16 0x4deb85 in main /home/jkeenan/gitwork/perl/perlmain.c:139:18
    #17 0x7f5718f8fd09 in __libc_start_main csu/../csu/libc-start.c:308:16

SUMMARY: AddressSanitizer: 576 byte(s) leaked in 2 allocation(s).
EXIT STATUS: 1
# Failed test 14 - test from lib/croak/class at line 101 at lib/croak/class line 101
lib/croak.t .......... 184/215 #
# Note: 'run_multiple_progs' run has one or more failures
#        you can consider setting the environment variable
#        PERL_TEST_ABORT_FIRST_FAILURE=1 before running the test
#        to stop on the first error.
#
lib/croak.t .......... Failed 1/215 subtests 
	(less 3 skipped subtests: 211 okay)
re/pat.t ............. ok         
re/pat_psycho_thr.t .. ok     
re/pat_psycho.t ...... ok     

Test Summary Report
-------------------
lib/croak.t        (Wstat: 0 Tests: 215 Failed: 1)
  Failed test:  14
Files=4, Tests=1475, 96 wallclock secs ( 0.29 usr  0.03 sys + 44.65 cusr 50.82 csys = 95.79 CPU)
Result: FAIL
Finished test run at Thu Feb 16 18:31:06 2023.

In my regular invocation of make test_harness, where TEST_JOBS=4, all 4 of the test files named above experienced failures.

[demerphq]

In my regular invocation of make test_harness, where TEST_JOBS=4, all 4 of the test files named above experienced failures.

I have to admit im baffled by this. Three of those tests are regex engine related, and seem to pass in the output you posted. Can you explain more? Should that even be in this ticket?

[jkeenan]

In my regular invocation of make test_harness, where TEST_JOBS=4, all 4 of the test files named above experienced failures.

I have to admit im baffled by this. Three of those tests are regex engine related, and seem to pass in the output you posted. Can you explain more? Should that even be in this ticket?

Based on what we have observed in #20810, when I ran the full make test_harness for the aforementioned build of perl (clang/ASAN/threads), I got errors in 4 test files. However, when I re-ran the just those 4 test files through the harness (and with TEST_JOBS=1), I only got the failures in t/lib/croak.t. So our finding of failures in that file is robust, whereas the failures in the other files are less so.

[demerphq]

Based on what we have observed in #20810, when I ran the full make test_harness for the aforementioned build of perl (clang/ASAN/threads), I got errors in 4 test files.

Do you think you could redo that from before when @leonerd merged the class stuff?Which I guess would be 5d4d8b9

If those test files fail at that commit ID under the build mode we are discussing here i think you should file a ticket for them specifically so I can look into it at some point.

[leonerd]

So, there's still one more memory leak left, but that's only apparent if a compiletime error causes a field expression inside a class block to fail its out-of-block controlflow test.

Compare:

$ PERL_DESTRUCT_LEVEL=2 valgrind --leak-check=full ./perl -I. -Mfeature=class -E 'class XXX { BEGIN { die "Oopsie" } }'

$ PERL_DESTRUCT_LEVEL=2 valgrind --leak-check=full ./perl -I. -Mfeature=class -E 'class XXX { field $x; BEGIN { die "Oopsie" } }'

neither of which leak, with

$ PERL_DESTRUCT_LEVEL=2 valgrind --leak-check=full ./perl -I. -Mfeature=class -E 'class XXX { field $x = last; }'

which does. At that point I'm going to say yes, this is a valid bug. But I don't think it's as urgent to fix before 5.37.9's release - especially as I am going on vacation tomorrow. I would verymuch like to get this PR here merged first, because this PR fixes such bugs as "every single ->new'ly allocated object will leak an entire C array". A slightly bigger bug ;)

This PR already has one approval, so if nobody else shouts in the next 3 hours or so, I'm going to merge it anyway on the grounds that fixing 5 bugs while 1 still remains is better than not fixing any. When I do I'll open another Issue to track the remaining problem.