Page MenuHomePhabricator

[WIP] Fix member access of anonymous struct/union fields in C
AcceptedPublic

Authored by aaron.ballman on May 7 2022, 7:39 AM.

Details

Reviewers
rsmith
nickdesaulniers
erichkeane
jyknight
Group Reviewers
Restricted Project
Summary

We were accepting invalid code where the qualifiers of the anonymous structure were not taken into account when forming a member access to one of the indirect fields, as in:

struct S {
  const struct {
    int i;
  };
} s;

void foo(void) {
  s.i = 12; // previously accepted, now rejected
}

We now expose the anonymous structure's field as having a qualified type instead of an unqualified type so that access checking notes that an intermediate object in the expression is qualified. We also adjusted the diagnostic so that it's slightly more user friendly despite this being a rather rare case.

Note, this only impacts C; in C++ and with the Microsoft C anonymous structure extension, the qualifiers are stripped.

Fixes #48099

Diff Detail

Event Timeline

aaron.ballman created this revision.May 7 2022, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 7:39 AM
aaron.ballman requested review of this revision.May 7 2022, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 7:39 AM
aaron.ballman retitled this revision from Fix member access of anonymous struct/union fields in C to [WIP] Fix member access of anonymous struct/union fields in C.May 7 2022, 8:37 AM

I'm marking this as a WIP -- the content is ready to go, but I'm no longer convinced we want to implement this for GCC compatibility as GCC is the only C compiler I can find that cares about the qualifiers: https://godbolt.org/z/hTqY8zMb5

tahonermann added inline comments.
clang/docs/ReleaseNotes.rst
152–157

Perhaps the first sentence should clarify that it applies only to C modes? Though the code changes don't appear to be specific to C.

I'm confused by "Note that qualifiers are ignored in C++ and for Microsoft's extension of anonymous objects". I find it very surprising that cv-qualifiers would be ignored in C++. Unless there is a relevant core issue?

clang/test/Sema/anonymous-struct-union.c
137–140

Top-level cv-qualifiers are ignored in generic selection expressions, so I'm not sure what these assertions are intended to ensure.

aaron.ballman added inline comments.May 9 2022, 9:10 AM
clang/docs/ReleaseNotes.rst
152–157

I can definitely clarify if we decide to go this route.

The C++ behavior is intentional (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L5352) but perhaps emergent as the C++ standard is silent on qualifiers on the anonymous union object, but all C++ implementations agree to ignore the qualifiers: https://godbolt.org/z/7Wd8Kn5Mf.

clang/test/Sema/anonymous-struct-union.c
137–140

I think this is the second time I've been caught by that. I keep expecting that the "no two associations can use compatible types" to make that situation an error.

I almost wonder if we should give a warning in cases where an association type is qualified (or is an array/function type rather than a decayed pointer)?

tahonermann added inline comments.May 9 2022, 9:19 AM
clang/test/Sema/anonymous-struct-union.c
137–140

I was surprised when I first learned of this behavior too; it hobbles the utility of generic selection expressions in some cases.

A warning like you suggest does seem appropriate. I'm not aware of any way in which a cv-qualified or non-decayed association type can be used in any useful manner (other than to show that a compiler is not behaving properly). If there is such a use, I would be interested in knowing what it is!

nickdesaulniers accepted this revision.May 9 2022, 12:10 PM

Consider adding an AST test; that was something that came up in my patch https://reviews.llvm.org/D95408.

I'm pushing back on the WG14 lists to see if this is a good opportunity to clarify the other direction and be compatible with C++ in the process.

It would really be good to have some clarification from WG14 added to the spec; as is, code is being written in the Linux kernel that uses this functionality to avoid marking all members of an anonymous aggregate as const. This is resulting in code being submitted where clang doesn't error on modification of such members, while GCC errors, so such patches are rejected. So developers using clang are getting caught off guard for a code base that needs to be compiler portable.

Kernel developers were of the opinion that it is nice syntactic sugar. For that reason, I agree with them. But I'm also fine leaving this out of clang until the spec is amended to clarify. That said, if WG14 doesn't pursue this (but doesn't reject or accept), I'd like to have this in clang sooner for better compatibility with GCC. It is too bad though if WG14 chooses to further diverge C from C++ over this...

This revision is now accepted and ready to land.May 9 2022, 12:10 PM