Make `struct shared_he` opaque

[happy-barney]

Making structures use internally to implement some internal behaviour opaque with accessor functions only makes code less fragile and makes related maintenance much easier.

Change doesn't affect language nor API.

[happy-barney]

Follow-up topics:

• big game change is to make struct hek opaque with modifications allowing int + str key

Identified issue: code base doesn't treat LTO inline using macro-altered syntax which will be as compiler independent as possible. currently there are two macros one should add to function prototype:

• __attribute__always_inline__ (used as "postfix" syntax)
• PERL_STATIC_FORCE_INLINE (used as "prefix" syntax)

Prefix - used before return type Postfix - used after arguments

I did some documentation research how various compilers marks function for LTO, summarized into table:

Compiler ID	LTO Since (Year)	LTO Since (Version)	Inline Syntax for LTO	Prefix	Postfix
GCC	2010	4.5	__attribute__((always_inline)) inline	Y	Y
Clang	2011	3.0	__attribute__((always_inline)) inline	Y	Y
MSVC	2015	Visual Studio 2015	__forceinline	Y	-
Intel C++	2008	11.0	__attribute__((always_inline)) inline	Y	Y
			__forceinline	Y	-
IBM XL C	2012	13.1	#pragma inline(always)	Y	-
			__attribute__((always_inline)) inline	Y	Y
Oracle Studio	2012	Solaris Studio 12.3	__inline	Y	-

Suggestion:

• create new macro PERL_ALWAYS_INLINE, for prefix usage (better name suggestions are welcome)
• remove usage of old macros

I can create POC PR for this

[mauke]

Similar, but with a lot less casting:

diff --git hv.c hv.c
index 6fea67d7af..ec7df5a8bb 100644
--- hv.c
+++ hv.c
@@ -177,6 +177,24 @@ S_new_he(pTHX)
 
 #endif
 
+struct shared_he {
+    struct he shared_he_he;
+    struct hek shared_he_hek;
+};
+
+struct hek *
+Perl_share_hek_hek(const struct hek *hek)
+{
+    assert(hek != NULL);
+    struct shared_he *const he = (struct shared_he *)(
+        (unsigned char *)hek
+        - STRUCT_OFFSET(struct shared_he, shared_he_hek)
+    );
+    assert(&he->shared_he_hek == hek);
+    he->shared_he_he.he_valu.hent_refcount++;
+    return &he->shared_he_hek;
+}
+
 STATIC HEK *
 S_save_hek_flags(const char *str, I32 len, U32 hash, int flags)
 {
diff --git hv.h hv.h
index a6319cd60a..251ada6ea0 100644
--- hv.h
+++ hv.h
@@ -63,10 +63,7 @@ struct hek {
        is UTF-8 or WAS-UTF-8, or an SV */
 };
 
-struct shared_he {
-    struct he shared_he_he;
-    struct hek shared_he_hek;
-};
+struct shared_he;
 
 /* Subject to change.
    Don't access this directly.
@@ -520,12 +517,8 @@ whether it is valid to call C<HvAUX()>.
 #define Perl_sharepvn(pv, len, hash) HEK_KEY(share_hek(pv, len, hash))
 #define sharepvn(pv, len, hash)	     Perl_sharepvn(pv, len, hash)
 
-#define share_hek_hek(hek)						\
-    (++(((struct shared_he *)(((char *)hek)				\
-                              - STRUCT_OFFSET(struct shared_he,		\
-                                              shared_he_hek)))		\
-        ->shared_he_he.he_valu.hent_refcount),				\
-     hek)
+struct hek *Perl_share_hek_hek(const struct hek *) __attribute__nonnull__(1);
+#define share_hek_hek(hek) Perl_share_hek_hek(hek)
 
 #define hv_store_ent(hv, keysv, val, hash)				\
     ((HE *) hv_common((hv), (keysv), NULL, 0, 0, HV_FETCH_ISSTORE,	\

[happy-barney]

@mauke mergeable or more requests to change ?

[tonycoz]

code base doesn't treat LTO inline using macro-altered syntax which will be as compiler independent as possible. currently there are two macros one should add to function prototype:

* `__attribute__always_inline__` (used as "postfix" syntax)

* `PERL_STATIC_FORCE_INLINE` (used as "prefix" syntax)

We might have different understandings of "LTO" here.

To me that's "link time optimization" which is global optimization between compilation units, for example:

tony@venus:.../git/lto$ cat main.c
extern int square(int);

int main() {
  return square(3);
}
tony@venus:.../git/lto$ cat square.c
int square(int num) {
  return num * num;
}
                     
                    
tony@venus:.../git/lto$ gcc -O2 main.c square.c
tony@venus:.../git/lto$ objdump --disassemble=main a.out
...
0000000000001040 <main>:
    1040:       bf 03 00 00 00          mov    $0x3,%edi
    1045:       e9 f6 00 00 00          jmp    1140 <square>
...
tony@venus:.../git/lto$ gcc -O2 -flto main.c square.c
tony@venus:.../git/lto$ objdump --disassemble=main a.out
...
0000000000001040 <main>:
    1040:       b8 09 00 00 00          mov    $0x9,%eax
    1045:       c3                      ret
...

Note how -flto allows square() from the other compilation unit to be inlined.

AFAIK forcing inline doesn't do this - it would be unfriendly as link time optimization can greatly extend link times.

[happy-barney]

@tonycoz yes, maybe we did have ... force inline / always inline is a mechanism how to force inline function across modules - ie it is not LTO per se, rather forcing LTO to inline this function. Really relying on optimization will be to get rid of these force inline macros

[tonycoz]

The issues I see with this change:

• it turns a macro which compiles down to a single instruction into a function call. a -flto build with likely inline that call, but most people don't build with -flto
• a very small number of CPAN modules (B::C, Devel::Size, Devel::SizeMe) depend on the definition of shared_he which is now hidden.

[happy-barney]

The issues I see with this change:

* it turns a macro which compiles down to a single instruction into a function call.  a -flto build with likely inline that call, but most people don't build with -flto

This PR about opaque structures or not? Use advantages and accept consequences? It also may be sane to detect LTO and use it automatically

* a very small number of CPAN modules (B::C, Devel::Size, Devel::SizeMe) depend on the definition of shared_he which is now hidden.

Good point. I missed that (in fact used \b without proper quoting, shared_heb doesn't exist in metacpan-cpan-extracted) In case of going forward with opaque structures

• which to make opaques - registration data structures (eg struct mro_alg) should remain public
• phasing of open declaration

[tonycoz]

This PR about opaque structures or not? Use advantages and accept consequences?

The issue is whether it's worth opaquing this data structure.

I don't really have a strong opinion either way on this particular structure, but as much as I dislike how much the perl "API" exposes perl's internals, and the namespace pollution that produces[1], that openness has allowed CPAN do some fairly useful (and sometimes scary) things.

It also may be sane to detect LTO and use it automatically

I think that would be useful, though we'd want an option to disable it to allow for a faster edit/compile/link loop, and to deal with toolchains with broken lto.[2]

[1] try compiling C++ code that includes the perl headers first and then some C++ standard headers

[2] which appears to be the case in https://www.nntp.perl.org/group/perl.perl5.porters/2022/11/msg265099.html

[happy-barney]

This PR about opaque structures or not? Use advantages and accept consequences?

The issue is whether it's worth opaquing this data structure.

I don't really have a strong opinion either way on this particular structure

Problem with this particular structure is that it uses struct hek, which is one that should be opaque so it can evolve without breaking API. Namely I'd like to change it to:

struct hek {
    U32         hek_hash;        /* computed hash of key */
    I32         hek_len;        /* length of the hash key */

    U32        hek_flags;
    union    {
        char str[1];
        void *ptr;
    } hek_key;
}

... which will be another complex task on its own

so regarding all mentioned here, in case of adapting opaque structs, suggestion of how to:

• define macro, for example PERL_OPAQUE_STRUCT_SHARED_HE
• define reasonable (or all?) accessors, all with Perl_ prefix (starting as static inline in headers?)
• CPAN contribution: direct access to struct properties should be replaces with conditional based on opaque macro
• struct will be still public, slowly phasing into opaque

[tonycoz]

Namely I'd like to change it to:

struct hek {
    U32         hek_hash;        /* computed hash of key */
    I32         hek_len;        /* length of the hash key */

    U32        hek_flags;
    union    {
        char str[1];
        void *ptr;
    } hek_key;
}

I'm not sure that's valid.

As you know, the trailing char[1] in a HEK is a pre C99 way of doing C99's flexible array member, and I expect many compilers support that for just that usage.

Putting that inside a union may not be supported though (gcc does though) and isn't required by C99 for the standard flexible array members.

The void * will also add four bytes of padding between hek_flags and hek_key on most 64-bit platforms.

[tonycoz]

The void * will also add four bytes of padding between hek_flags and hek_key on most 64-bit platforms.

Note that you could just allocate a sizeof(void *) buffer and use accessors like we use for ARGp in regcomp.h:

static inline SV *
ARGp_VALUE_inline(struct regnode *node) {
    SV *ptr;
    memcpy(&ptr, ARGp_BYTES_LOC(node), sizeof(ptr));

    return ptr;
}

static inline void
ARGp_SET_inline(struct regnode *node, SV *ptr) {
    memcpy(ARGp_BYTES_LOC(node), &ptr, sizeof(ptr));
}

On systems that support misaligned access these optimize to single instructions, others may do the copy or special unaligned access instructions.

I do wonder what the void * will be used for.

[happy-barney]

@tonycoz I do know that hek_key currently contains additional \0 and additional char (flags), or that it allocates sizeof struct + some

void * is there to force alignment, which current comment mentioned. If that's not a case anymore, then we can get rid of void * and comment at all

[bulk88]

I disagree with these changes, just rename the struct with some more letters or "_"s. Each new inline function can fail to inline at compiler discretion. Or ELF/LD extern symbol rules or .o file format rules break the inlining. And LTO working perfectly on all CCs on all platforms is rare. Or with C++ compilers, and their type mangling, you created 100s of copies of "struct shared_he" with names like "struct shared_he_1729190497" and then things go sour at link time (template explosion problems).

Plus CPAN/XS should be able to fix problems in the core interp, after the core interp is stable/gold released or outside P5P support life cycle.

perl is not a commercial proprietary application and this commit against the ethos of open source.