Make globals with mutable members non-constant, even in custom sections

[dblaikie]

Summary

Turned out we were making overly simple assumptions about which sections (& section flags) would be used when emitting a global into a custom section. This lead to sections with read-only flags being used for globals of struct types with mutable members.

Fixed by porting the codegen function with the more nuanced handling/checking for mutable members out of codegen for use in the sema code that does this initial checking/mapping to section flags.

[rnk]

clang/lib/Sema/SemaDecl.cpp

14363

I think this is not compatible with MSVC. MSVC uses simple logic, it doesn't look for mutable: https://gcc.godbolt.org/z/sj6d4saxx

The const mutable struct appears in the myrdata section in that example.

I think the solution is to separate the flag logic from the pragma stack selection logic, which has to remain MSVC-compatible.

[efriedma]

clang/lib/Sema/SemaDecl.cpp

14363

MSVC apparently looks at whether the variable is marked "const", and nothing else; it doesn't look at mutable, it doesn't look at whether the variable has a constant initializer. So the current code isn't right either; if we're trying to implement MSVC-compatible logic, we shouldn't check HasConstInit.

That said, I'm not sure how precisely/in what modes we want to precisely emulate MSVC. Probably anything we do here will be confusing.

[rsmith]

clang/lib/Sema/SemaDecl.cpp

14363

We should at least issue a warning if the sensible logic and the MSVC-compatible calculation differ. @rnk, do you know how important it is to follow the MSVC semantics in this regard?

[rnk]

clang/lib/Sema/SemaDecl.cpp

14363

I think it depends on whether you think that users are primarily using these pragmas to override the default rdata/bss/data sections without any care about precisely what goes where, or if they are using them to do something finer grained.

If I had to guess, I'd say it's more likely the former, given that __declspec(allocate) and #pragma(section) exist to handle cases where users are putting specific globals into specific sections.

Which, if we follow Richard's suggestion, would mean warning when we put a global marked const into a writable section when ConstSegStack is non-empty. That seems reasonable. -Wmicrosoft-const-seg for the new warning group?

[dblaikie]

clang/lib/Sema/SemaDecl.cpp

14363

Does the MSVC situation only apply to custom sections? (presumably when not customizing the section, MSVC gets it right and can support a const global with a runtime initializer, mutable member, or mutating dtor?)

I think this code still needs to be modified, since this is the code that drives the error about incompatible sections. So it'll need to behave differently depending on the target platform?

[efriedma]

clang/lib/Sema/SemaDecl.cpp

14363

Yes, the MSVC situation is specifically if you specify #pragma const_seg; without the pragma, it does what you'd expect.

[dblaikie]

Add a warning for MSVC pragma inconsistency

[dblaikie]

clang/lib/Sema/SemaDecl.cpp

14363

Went with the "let's do the thing that the user probably wants, but isn't what MSVC does, and warn when that difference comes up" - if that's OK with everyone.

(always open to wordsmithing the warning - and if we want to, can go to the extra layer and specifically diagnose which reason (mutable members, non-const init) - and I can't quite figure out the best phrasing to say "we're putting it in section X insetad of section Y, because Z, but Microsoft would use X because A" or something... it's all a bit of a mouthful)

[dblaikie]

@rnk & I figured that the user probably isn't relying on this much - if they cared about a particular variable, they could use an attribute instead of a pragma. The pragma just being about "put all my const things over here and all my non-const things over there" - bug-for-bug compatibility doesn't seem /as/ needed here.

Not sure how you feel about it, @efriedma ?

[efriedma]

Sure, diverging from MSVC here seems fine. I agree it's unlikely anyone would actually want to put a variable that will be modified in a "const" section.

clang/lib/Sema/SemaDecl.cpp

14363

Describing which reason actually applies would make the warning a lot easier to read.

[rnk]

Thanks! My concerns are addressed, but please confirm that Eli's are too.

clang/lib/Sema/SemaDecl.cpp

14363

That is true, but I think very few people will see this diagnostic. I'm not sure it's worth the added code complexity to implement that improvement.

[dblaikie]

Diagnose specific reasons

clang/lib/Sema/SemaDecl.cpp

14363

Updated with a specific diagnosis. Phrasing still feels a bit awkward/I'm open to wordsmithing suggestions.

[efriedma]

LGTM

[eddyz87]

Hi @dblaikie,

After this revision landed yesterday one BPF kernel selftest (written in C) stopped building. I narrowed the issue to the following example:

#define SEC(n) __attribute__((section(n)))

const int with_init SEC("foo") = 1;
const int no_init SEC("foo");

It fails with the following error:

$ clang -c t.c -o -
t.c:4:11: error: 'no_init' causes a section type conflict with 'with_init'
    4 | const int no_init SEC("foo");
      |           ^
t.c:3:11: note: declared here
    3 | const int with_init SEC("foo") = 1;
      |           ^
1 error generated.

The error occurs because clang infers PSF_Read attribute for section "foo" when with_init is processed and PSF_Read | PSF_Write attributes for section "foo" when no_init is processed, thus reporting diag::err_section_conflict. The behavior changed because of the following lines introduced in this revision:

void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
  ...
  if (GlobalStorage && var->isThisDeclarationADefinition() &&
      !inTemplateInstantiation()) {
    ...
    if (var->hasInit() && HasConstInit && !(Reason =
        var->getType().isNonConstantStorage(Context, true, false))) {
      Stack = &ConstSegStack;
    } else {
      SectionFlags |= ASTContext::PSF_Write;
      Stack = var->hasInit() && HasConstInit ? &DataSegStack : &BSSSegStack;
    }
    ...
  }
  ...
}

Because of the var->hasInit() check any global w/o initializer gets a PSF_Write flag, which was not the case before this revision. So, now if one wants to have some const globals in the same section, these globals need to have an initializer.

I checked C language standard (N3088, section "6.7.3 Type qualifiers") and it does not offer much to tell if my example is correct or not, so I guess it is implementation dependent. GCC accepts my example w/o errors or warnings.

So, a question: might it be the case that var->hasInit() check is too restrictive?
(I do understand that w/o some custom linker behavior const globals w/o initializer don't make much sense).

[eddyz87]

As an additional data point, the same example but w/o section specification compiles fine:

const int with_init = 1;
const int no_init;

And puts both globals to the same section:

$ clang -c t.c -o - | llvm-readelf --section-headers -s -
Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  ...
  [ 3] .rodata           PROGBITS        0000000000000000 000040 000008 00   A  0   0  4
  ...

Symbol table '.symtab' contains 4 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     ...
     2: 0000000000000000     4 OBJECT  GLOBAL DEFAULT     3 with_init
     3: 0000000000000004     4 OBJECT  GLOBAL DEFAULT     3 no_init

(Ndx stands for section Nr).

[dblaikie]

In D156726#4591543, @eddyz87 wrote:

As an additional data point, the same example but w/o section specification compiles fine:

const int with_init = 1;
const int no_init;

And puts both globals to the same section:

$ clang -c t.c -o - | llvm-readelf --section-headers -s -
Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  ...
  [ 3] .rodata           PROGBITS        0000000000000000 000040 000008 00   A  0   0  4
  ...

Symbol table '.symtab' contains 4 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     ...
     2: 0000000000000000     4 OBJECT  GLOBAL DEFAULT     3 with_init
     3: 0000000000000004     4 OBJECT  GLOBAL DEFAULT     3 no_init

(Ndx stands for section Nr).

Thanks for the details - looking into it!

[dblaikie]

Fixed in 19c216b5ab7c34c297b4c3c4da64b49b3c830a3a

[yonghong-song]

In D156726#4593671, @dblaikie wrote:

Fixed in 19c216b5ab7c34c297b4c3c4da64b49b3c830a3a

Hi, @dblaikie I cannot find the above commit in llvm-project and with latest llvm-project 'main' branch, the problem seems still exist.

[dblaikie]

In D156726#4594005, @yonghong-song wrote:

In D156726#4593671, @dblaikie wrote:

Fixed in 19c216b5ab7c34c297b4c3c4da64b49b3c830a3a

Hi, @dblaikie I cannot find the above commit in llvm-project and with latest llvm-project 'main' branch, the problem seems still exist.

Ah, right right - sorry, failed to push. Here it is: 2993243c45abdb4f2bc3979336d054be165b1134

[yonghong-song]

Ah, right right - sorry, failed to push. Here it is: 2993243c45abdb4f2bc3979336d054be165b1134

Thanks! I tested and the problem with bpf backend is resolved!