This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dblaikie on Jul 31 2023, 11:46 AM.

Details

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.

Diff Detail

Event Timeline

dblaikie created this revision.Jul 31 2023, 11:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 11:46 AM
dblaikie requested review of this revision.Jul 31 2023, 11:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 11:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added a subscriber: rnk.Jul 31 2023, 12:52 PM
rnk added inline comments.
clang/lib/Sema/SemaDecl.cpp
14254

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 added inline comments.Jul 31 2023, 2:08 PM
clang/lib/Sema/SemaDecl.cpp
14254

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 added a subscriber: rsmith.Aug 2 2023, 10:02 AM
rsmith added inline comments.
clang/lib/Sema/SemaDecl.cpp
14254

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 added inline comments.Aug 2 2023, 10:32 AM
clang/lib/Sema/SemaDecl.cpp
14254

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 added inline comments.Aug 9 2023, 3:21 PM
clang/lib/Sema/SemaDecl.cpp
14254

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 added inline comments.Aug 9 2023, 3:40 PM
clang/lib/Sema/SemaDecl.cpp
14254

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

dblaikie updated this revision to Diff 549206.Aug 10 2023, 5:11 PM

Add a warning for MSVC pragma inconsistency

dblaikie added inline comments.Aug 10 2023, 5:19 PM
clang/lib/Sema/SemaDecl.cpp
14254

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)

@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 ?

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
14254

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

rnk accepted this revision.Aug 11 2023, 3:49 PM

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

clang/lib/Sema/SemaDecl.cpp
14254

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.

This revision is now accepted and ready to land.Aug 11 2023, 3:49 PM
dblaikie updated this revision to Diff 550097.Aug 14 2023, 2:48 PM

Diagnose specific reasons

clang/lib/Sema/SemaDecl.cpp
14254

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

This revision was landed with ongoing or failed builds.Aug 14 2023, 3:26 PM
This revision was automatically updated to reflect the committed changes.

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).

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).

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!

Fixed in 19c216b5ab7c34c297b4c3c4da64b49b3c830a3a

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.

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

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

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