class.c: clean up any state if we don't finish the class #22374

tonycoz · 2024-07-04T00:22:46Z

class.c

jkeenan · 2024-08-26T13:50:11Z

@leonerd, could you please review this p.r. from @tonycoz? Thanks.

bulk88 · 2024-10-17T19:32:36Z

class.c

+    struct xpvhv_aux *aux = HvAUX(stash);
+
+    SvREFCNT_dec(aux->xhv_class_superclass);
+    aux->xhv_class_superclass = NULL;


I suggest lifting the pointers to a C autos, and setting struct field to NULL, then call SvREFCNT_dec. Analyse if SvREFCNT_dec_NN is appropriate.

xhv_class_superclass can be NULL if the class has no base class defined.

I don't think there's any value re-ordering here, since we only write to xhv_class_superclass after the REFCNT_dec.

I think it's unlikely the cache line will expire - this is a reference to an existing class, so it's just going to reduce the reference count and test it.

My rational for the change is so the CC has the opportunity to not write the int literal that is part of ->xhv_class_superclass 2x in machine code. I doubt the offset of ->xhv_class_superclass is 0x0. Its more likely to be 0x18 or 0x30 or 0x10. Foo-arch RISC CPUs may or may not not allow writing a +/- (U8) 0xF0 literal, that will be embedded inside the ld/st/mov opcode. So on Foo RISC CPU arch, Either the user wants load/store opcodes, or the user wants integer algebra opcodes. Can't have your cake and eat it too,

I have a personal policy to write C code, pretending that certain Intel x86 only op codes do not exist in my head, and to avoid writing C code, that will emit those certain Intel x86 only op,

Specifically I try to not use Intel's lea op, which is unique on planet earth. No other CPU has ever had an op that does a1 = a1+a2+(a3<<a4) ; AND that op DOES NOT CHANGE EFLAGS/jmp status control bits,

I also avoid Intel's swiss army knife of pre-calculus math exams, the 2 byte ling SIB operand specifier. I prefer to stick to old fashioned 1 byte long modR/M operands.

lea and sib is easily fixed by just factoring out some math from your struct deref statements in your inner loop.
Change int i = 0; i++; to using a real pointer, that gets bumped by sizeof(c_struct) and < compare against a read END pointer in your loop. All those LEA and SIB ops fluffery has been factored out by hand in C pretty easily.

ARM32 CPUs can't store more than a U8 literal in THUMB machine code, and can't store more than a 2^12 (4096) integer literal operand in 4byte OPs ARM. All C lang literals > 0x100 or > 4096, become separate instruction LOAD DblWrdRegX2, *(U32*)(IP+(0xff*4); opcodes, Seeing all those >0x80 >0x100 >0x1000 src code literal ints/src code struct derefs on large structs, turning to separate mov opcodes, to GET MY OWN CONST LITERALS THAT I TYPED IN BASE 10!!! It makes my stomach feel sick reason Arm32 asm code,

So to solve every possible flaw and defect,, on every CPU arch, for all CPU archs past present and future, on every commercial grade and hobby grade C compiler from any vendor, combine whit any CC -O -m -arch cmd line args. and so I know I wrote good clean code and I dont need to decompile it because I can trust that the CC, any CC, I used, it did the best choice that I wanted.

So solve all over that, just lift the ptr_to_dotr from malloc mem first, to a C auto/volatile register, assign NULL to the malloc mem you just lifted, then pass [IRL there is no"pass"its already inside RCX or RDI] the lifted ptr stored in a volatile register to the heavy weight C and invoke the heavy weight C call.

benefit is minimal phy mem interactions, minimum computing abs addrs of data vars from offsets into structs.

3 aux->xhv_class_superclass statements become

void ** p = &aux->xhv_class_superclass; if(*p) { void * p2 = *p; *p = NULL; foo_free(p2); }

not

if(aux->xhv_class_superclass) foo_free(aux->xhv_class_superclass); aux->xhv_class_superclass = NULL;

What if var aux happens to be a c stack var and not a non vol register? Maybe foo_free() changed the ptr inside C stack var aux that was stored/splilled to phy vir memory? escape analysis? How do you know what the the e-liberal/e-conservative/e-communist/e-fascist parties at IEEE going to vote on for next month's draft spec of https://developers.redhat.com/articles/2022/06/02/use-compiler-flags-stack-protection-gcc-and-clang#safestack_and_shadow_stack ?

so worst case possible, aux->xhv_class_superclass = NULL; has 2 read ops, 1 xor op, 1 write op. my perfect version eliminated the 2 read ops,
earlier by doing

void ** p = &aux->xhv_class_superclass; if(*p) {

bulk88 · 2025-01-13T22:18:34Z

class.c

+            if (SvTYPE(entry) == SVt_PVGV
+                && (cv = GvCV((GV*)entry))
+                && (CvIsMETHOD(cv) || memEQs(kpv, klen, "new"))) {
+                SvREFCNT_dec(cv);


Add _NN(), cv already proved to be derefable

bulk88 · 2025-01-13T22:21:25Z

class.c

+            }
+            else if (SvTYPE(entry) == SVt_PVCV
+                     && (CvIsMETHOD((CV*)entry) || memEQs(kpv, klen, "new"))) {
+                (void)hv_delete(stash, kpv, HeUTF8(he) ? -(I32)klen : (I32)klen,


Pass the HEK* maybe, or U32 hash. hv_delete_ent() maybe??? XS api is not fully there for exotic with HEK with hash, not fetch, HV operations.

hv_delete_ent() would require making a SV, this would only be efficient for the (I think) atypical case of the key being an SVKEY.

hv_delete_ent() would require making a SV, this would only be efficient for the (I think) atypical case of the key being an SVKEY.

# define hv_deletehek(hv, hek, flags) \ hv_common((hv), NULL, HEK_KEY(hek), HEK_LEN(hek), HEK_UTF8(hek), \ (flags)|HV_DELETE, NULL, HEK_HASH(hek))

usng this vs hv_delete() is still a better choice, U32 has wasnt recalculated.

Unrealistic bc there arent supposd 2 exit outside TIEHASH

#define HePV(he,lp) ((HeKLEN(he) == HEf_SVKEY) ? SvPV(HeKEY_sv(he),lp) :((lp = HeKLEN(he)), HeKEY(he)))

If santa says youver been a good boy HeKEY_sv(he) will be a SVLEN()==0 HEK* COW. Note either way, you get 2 accelerated hash lookup data structures here, either an unreliatic SV*, or a HEK * from shared HEK POOL. Its wrong to use const RO "CString" hv_delete() here, you have the meta data to do something better,

bulk88 · 2025-01-13T22:23:10Z

class.c

+
+    /* remove any ISA entries */
+    SV *isaname = newSVpvf("%" HEKf "::ISA", HvNAME_HEK(stash));
+    sv_2mortal(isaname);


sv_2mortal(isaname); to isaname = sv_2mortal(isaname);, use the free retval register.

bulk88 · 2025-01-13T22:29:18Z

class.c

-                newSVOP(OP_CONST, 0, (SV *)superaux->xhv_class_initfields_cv),
-                NULL);
+        U32 fieldix = PadnameFIELDINFO(pn)->fieldix;
+        (void)hv_store_ent(fieldix_to_padix, sv_2mortal(newSVuv(fieldix)), newSVuv(padix), 0);


is there a better way of doing this other than a trip through SvPV(3args)'s S_uiv_2buf(), or worse going to libc sprintf with dozens of lock aq/releases, and maybe some kernel calls to the FS. I would just have 4 byte / 8 byte UVs stored straight as raw unprintable binary with SvPOK_on(). Hash algo/perl just sees char strings, vs turning them into longer less dense ascii bytes.

This code was only re-indented

bulk88 · 2025-01-13T22:34:22Z

class.c

+        HE *he;
+        if((he = hv_fetch_ent(fieldix_to_padix, sv_2mortal(newSVuv(fieldix)), 0, 0)) &&
+           SvOK(HeVAL(he))) {
+            fieldop->op_targ = SvUV(HeVAL(he));


factor out HeVAL(he), get the SV*. its too many layers deep of derefs in code, and probably does optimize, but its 3 or 4 derefs now from the C auto. if accidents happen with multi eval, they will be smaller.

This code was only re-indented

bulk88 · 2025-01-13T22:35:43Z

class.c

+        fieldop->op_private = op_priv;
+
+        HE *he;
+        if((he = hv_fetch_ent(fieldix_to_padix, sv_2mortal(newSVuv(fieldix)), 0, 0)) &&


idk if this is a loop, but if it is, y not reuse the sv_2mortal(newSVuv()); ?

It is a loop, but this code was only re-indented.

jkeenan · 2025-03-19T20:56:28Z

@tonycoz do you have a feeling for whether or not we're going to be able to merge this pull request into blead in this dev cycle or not? If not, then I recommend we label it 'defer-next-dev'.

tonycoz · 2025-03-19T23:34:22Z

It's not going anywhere until @leonerd looks at it

ap · 2025-04-24T16:43:50Z

@leonerd Ping?

leonerd · 2025-04-24T17:06:08Z

t/class/gh22169.t

+    field $x = "First";
+    field $w :reader;
+    ADJUST {
+        fail("ADJUST from failed class definition called");


Likely wants to be ::fail()

Fixed, thanks.

leonerd

Code addition looks fine.
I haven't read in detail the large changes to Perl_class_seal_stash() but I presume this is mostly just a re-indentation exercise with no change besides invoking the new cleanup function in the failure case.

I have read the new .t file about four times and I imagine it's supposed to invoke a compiletime error in the eval STR but I can't see what the error is supposed to be. Could it be made clearer there?

tonycoz · 2025-04-27T23:18:12Z

I haven't read in detail the large changes to Perl_class_seal_stash() but I presume this is mostly just a re-indentation exercise with no change besides invoking the new cleanup function in the failure case.

Yes, Except for some re-wrapping it's just indentation, the github diff viewer has an option to ignore whitespace which is handy for changes like this:

tonycoz · 2025-04-27T23:22:04Z

I have read the new .t file about four times and I imagine it's supposed to invoke a compiletime error in the eval STR but I can't see what the error is supposed to be. Could it be made clearer there?

There's no closing }, I'll add a comment.

Fixes Perl#22169

tonycoz · 2025-04-28T00:22:08Z

Squashed the two commits since it didn't seem worth messing with adding to the first commit and dealing with the conflicts in moving the code to the new function.

leonerd

Ah yes, that test makes sense now. LGTM

tonycoz requested a review from leonerd July 4, 2024 00:22

mauke reviewed Jul 4, 2024

View reviewed changes

class.c Outdated Show resolved Hide resolved

tonycoz force-pushed the 22169-class-cleanup branch from 977ed47 to 177bd8b Compare July 8, 2024 00:29

jkeenan added the class Issues related to 'class' keyword or __CLASS__ label Jul 9, 2024

bulk88 reviewed Oct 17, 2024

View reviewed changes

tonycoz mentioned this pull request Nov 28, 2024

deparse doesn't handle classes #22777

Open

bulk88 reviewed Jan 13, 2025

View reviewed changes

tonycoz added the defer-next-dev This PR should not be merged yet, but await the next development cycle label Mar 19, 2025

leonerd reviewed Apr 24, 2025

View reviewed changes

leonerd requested changes Apr 24, 2025

View reviewed changes

class.c: clean up any state if we don't finish the class

32bd2da

Fixes Perl#22169

tonycoz force-pushed the 22169-class-cleanup branch from c4e20ea to 32bd2da Compare April 28, 2025 00:18

leonerd approved these changes Apr 28, 2025

View reviewed changes

ap merged commit 8cd9e2a into Perl:blead Jun 19, 2025
33 checks passed

class.c: clean up any state if we don't finish the class #22374

class.c: clean up any state if we don't finish the class #22374

Conversation

tonycoz commented Jul 4, 2024

Uh oh!

Uh oh!

jkeenan commented Aug 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkeenan commented Mar 19, 2025

Uh oh!

tonycoz commented Mar 19, 2025

Uh oh!

ap commented Apr 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonerd left a comment

Choose a reason for hiding this comment

Uh oh!

tonycoz commented Apr 27, 2025

Uh oh!

tonycoz commented Apr 27, 2025

Uh oh!

tonycoz commented Apr 28, 2025

Uh oh!

leonerd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!