Fix offset detection in attr_subsys_construct

[devreal]

It appears that GCC starting from GCC 11 optimizes out the offset detection. By moving the pointers used for testing out of the function into TU scope we can suppress that optimization.

I have to admit that I am not sure I understand why we do this detection in the first place. There might be a better fix so I'm putting this PR out for review and discussion.

Fixes #10339

Signed-off-by: Joseph Schuchart [email protected]

[jeffhammond]

This makes the code thread-unsafe, does it not? That seems unacceptable. There must be a better solution.

[jeffhammond]

The following is a smaller change, and permits const on the static data, which seems safer.

diff --git a/ompi/attribute/attribute.c b/ompi/attribute/attribute.c
index c46b913f6d..2f3a63cc36 100644
--- a/ompi/attribute/attribute.c
+++ b/ompi/attribute/attribute.c
@@ -581,7 +581,7 @@ int ompi_attr_put_ref(void)
 static void attr_subsys_construct(attr_subsys_t *subsys)
 {
     int ret;
-    void *bogus = (void*) 1;
+    static const void *bogus = (void*) 1;
     int *p = (int *) &bogus;

     subsys->keyval_hash = OBJ_NEW(opal_hash_table_t);

[jeffhammond]

This still feels like the wrong answer though, since I can't help but wonder if this code is UB and GCC is right to do horrible things to it.

[jeffhammond]

03d9750 and 3e6c5bb are the only two commits that could have caused the problem.

[ggouaillardet]

Where did you get the impression this made the code non thread safe?
The change only impacts the scope of some read-only data.

[jeffhammond]

Static globals are not thread safe unless constant. If constant, add that to declaration, as I showed already in my proposed patch.

[devreal]

Working on an implementation that uses a union instead, to avoid this nail-curling mess. Will report back.

[ggouaillardet]

@devreal do we have to compute *_pos programmatically at runtime?
I mean could we simply macro define these values based on pointer sizes and endianness that are known at runtime?

[devreal]

I don't think we should keep this implementation, even if we could figure out the offsets without causing UB. It just seems awfully shaky. A much cleaner way is to use C unions and select the right member based on the set_from field. II put up a PR at #10344.

[devreal]

Superseded by #10344, closing.