Wrong deprecation warning of @system variable usage under CTFE

[dlangBugzillaToGithub]

Luís Ferreira reported this on 2024-11-19T15:17:00Z

Transferred from https://issues.dlang.org/show_bug.cgi?id=24867

CC List

• Dennis
• kinke

Description

```
align(1) struct Bar {
	align(1) const char* name;
}

struct Foo {
    static immutable Bar bar = Bar("foo");
    
    @safe
    void foo()
    {
        static assert(bar == "foo");
    }
}
```

This raises a deprecation warning, where it shouldn't. This is not a variable but a constant, its not GC tracked, so alignment doesn't particularly matter.

[dlangBugzillaToGithub]

contact commented on 2024-11-19T15:21:34Z

Sorry, example is also wrong. Consider the following:

```
align(1) struct Bar {
	align(1): 
    const char* name;
    int b = 2;
}

struct Foo {
    static immutable Bar bar = Bar("foo");
    
    @safe
    void foo()
    {
        static assert(bar.b == 2);
    }
}
```

[dlangBugzillaToGithub]

contact commented on 2024-11-19T15:26:17Z

There's a few flaws/improvements to make here:
1. Misaligned pointers in static immutables that are already initialised are fine
2. Usage of @system variables in CTFE may be ok (could be implementation defined, e.g. JIT-based CTFE may also use misaligned pointers, but probably can be eased on the current implementation)
3. Access of non-@system part of a struct should be @safe.

[dlangBugzillaToGithub]

dkorpel commented on 2024-11-19T16:05:17Z

Consider the same example with a different safety error, reading overlapped pointers:

```
union Bar 
{
    string name;
    int b;
}

static immutable Bar bar = Bar("foo");

@safe
void foo()
{
    static assert(bar.name == "foo");
}
```

Do you consider this a bug as well, because in the current AST-interpreter CTFE implementation, unions don't actually overlap?

CTFE should not be a separate language IMO. I don't like the disrepancies with 'runtime execution' that are already there, and I'd like to close the gap rather than widen it.

[dlangBugzillaToGithub]

kinke commented on 2024-11-19T16:27:40Z

It needs to be stressed that the deprecation comes from the **outer** `align(1)`, which is IMO a total anti-pattern anyway.

[dlangBugzillaToGithub]

contact commented on 2024-11-19T18:54:50Z

> It needs to be stressed that the deprecation comes from the **outer** `align(1)`, which is IMO a total anti-pattern anyway.

It's clear to me that blindly doing align(1) everywhere is an anti-pattern, but its not an always bad situation, there's places you really need to do this, e.g. implementing a protocol, dealing with raw data emplacement, etc. And I'm pretty sure you are well aware of those use-cases. 

Btw, we use it a lot, in traces protocol and probably also on the filesystem code, which, neither of those are ever tracked by the GC.

Therefore, it doesn't mean it shouldn't be supported. Anyway, I guess I don't have to explain it, but did it anyway.

> Do you consider this a bug as well, because in the current AST-interpreter CTFE implementation, unions don't actually overlap?

I think we should clarify the union situation and possibly make that specific case implementation defined, because someone might implement it with target machine code, like I said with JIT, although, I wouldn't recommend trusting a compiler that does JIT by escaping the safety semantics and therefore the sandbox that is CTFE, not only because of unpredictable output by relying on impure/poisoned state but also security-wise.

But I think **IT IS** definitely a bug for the case where it's guaranteed to be initialised as `.rodata`, regardless of being "misaligned" in the context of GC or not. I can't think of a way to be unsafe here. Also, the compiler can already easily check for this information.

[dlangBugzillaToGithub]

kinke commented on 2024-11-19T19:10:01Z

(In reply to Luís Ferreira from comment #5)
> > It needs to be stressed that the deprecation comes from the **outer** `align(1)`, which is IMO a total anti-pattern anyway.
> 
> It's clear to me that blindly doing align(1) everywhere is an anti-pattern,
> but its not an always bad situation, there's places you really need to do
> this, e.g. implementing a protocol, dealing with raw data emplacement, etc.
> And I'm pretty sure you are well aware of those use-cases. 
> 
> Btw, we use it a lot, in traces protocol and probably also on the filesystem
> code, which, neither of those are ever tracked by the GC.
> 
> Therefore, it doesn't mean it shouldn't be supported. Anyway, I guess I
> don't have to explain it, but did it anyway.

Any outer `align(1)` would never pass my code review - type alignments are okay for over-alignment, but not for under-alignment, those should always be field alignments. No example you gave here justifies the outer `align(1)`, the aggregates are all `align(1)` anyway.

[dlangBugzillaToGithub]

contact commented on 2024-11-19T20:12:52Z

> No example you gave here justifies the outer `align(1)`

It does. Considering our tracing system:

```
align(1) struct TracesMetadata {
align(1): 
    const char* name;
    int b = 2;
}

@section(".traces.metadata") @assumeUsed
static immutable Bar bar1 = Bar("foo1");
@section(".traces.metadata") @assumeUsed
static immutable Bar bar2 = Bar("foo2");
```

We need both alignment specifiers in order for the section to be fully packed across two of those constants. These items inside the traces metadata section shouldn't have any alignment, or, very specific alignment. The metadata is very much like DWARF `.debug_info`, not supposed to be used at runtime, so everything packed is desired to save space in the binary. Sure you can specify the alignment when you define the constant, but this is here to prevent forgetting and then having issues. 

These types are never ever used at the runtime of the program.

I can't give examples of filesystem, because I don't known enough about it to claim a point. Outer alignment, sure, it makes less sense, but it serves a purpose.

The packed outer alignment also makes sense in the context of embedded systems firmware where specific data types that are also often used inside sections are very space constraint (and not performance critical).

If you tell me a safety issue using these at the compile-time, sure, but forbidding it to be used within a safe context when its actually 100% safe, for me, it has no sense.

Btw, pedantic-wise, we shouldn't also specifically well-define this whole error at all, because this is only unsafe because of limitations or otherwise impractical behaviour of the druntime GC. I don't know how we phrase this in spec, but we should tell its implementation defined. If we take a look at DMD implementation, there's no reason to not allow it, because all the situations I showed above are 100% safe.

[dlangBugzillaToGithub]

contact commented on 2024-11-19T20:16:12Z

> Sure you can specify the alignment when you define the constant, but this is here to prevent forgetting and then having issues. 

Also, one more point on this: it doesn't matter where you put it if you, in the end, align it the same way, but the @system issue doesn't trigger on this situation:

```
struct Bar {
	align(1): 
    const char* name;
    int b = 2;
}

struct Foo {
    static immutable align(1) Bar bar = Bar("foo");
    
    @safe
    void foo()
    {
        static assert(bar.b == 2);
    }
}
```

[dlangBugzillaToGithub]

kinke commented on 2024-11-19T21:08:19Z

(In reply to Luís Ferreira from comment #7)
> > No example you gave here justifies the outer `align(1)`
> 
> It does. Considering our tracing system:
> 
> ```
> align(1) struct TracesMetadata {
> align(1): 
>     const char* name;
>     int b = 2;
> }
> 
> @section(".traces.metadata") @assumeUsed
> static immutable Bar bar1 = Bar("foo1");
> @section(".traces.metadata") @assumeUsed
> static immutable Bar bar2 = Bar("foo2");
> ```
> 
> We need both alignment specifiers in order for the section to be fully
> packed across two of those constants.

If that's really the case (with which compilers? and I assume `Bar` is `TracesMetadata` in your snippet), then that's a bug. The default aggregate alignment is the max of its members, so the common `align(1):` for all fields makes the aggregate automatically 1-aligned, and the outer `align(1)` type alignment *override* redundant [and IMO very bad practice].

All off-topic, I know, it's just that this deprecation can currently be worked around when dropping the type-align-override.