clang::Qualifiers::TQ flags are not in sync with DeclSpec::TQ flags

Clang Frontend
1

Hello,

clang::Qualifiers::TQ has 3 flags and CVRMask mask. Also it has the note:
// NOTE: These flags must be kept in sync with DeclSpec::TQ.

But
DeclSpec::TQ has 5 flags (so TQ_unaligned = 8, TQ_atomic = 16 are missed in clang::Qualifiers::TQ)

This inconsistency can be found by debugging test llvm/tools/clang/test/Sema/MicrosoftExtensions.c on the last declaration:
void test_unaligned2(int x[__unaligned 4]) {}

Array type is created incorrectly here, because '__unaligned' is lost in the ArrayType constructor at line:
ArrayTypeBits.IndexTypeQuals = tq;
tq has value 8 (TQ_unaligned) while IndexTypeQuals is declared to use ":3" bits only, so IndexTypeQuals is zero after assign.

Usually TQ fields are 5 bits (see i.e. DeclSpec::TypeQualifiers), so
declaration Type::ArrayTypeBitfields::IndexTypeQuals must be fixed as well

Hope it helps,
Vladimir.

2

Ping,

Could someone comment, please, if clang::Qualifiers::TQ intentionally reduces number of flags from 5 to 3?

Thanks,
Vladimir.

3

Ping,

Could someone comment, please, if clang::Qualifiers::TQ intentionally reduces number of flags from 5 to 3?

It is not intentional, no. We do not represent _Atomic as an ordinary qualifier in the AST, so I think this might accidentally be okay there. Offhand, I don't think we want to support either of those qualifiers as an array index type qualifier unless the C spec absolutely demands it.

John.

4

Ping,

Could someone comment, please, if clang::Qualifiers::TQ intentionally reduces number of flags from 5 to 3?

It is not intentional, no. We do not represent _Atomic as an ordinary qualifier in the AST, so I think this might accidentally be okay there. Offhand, I don't think we want to support either of those qualifiers as an array index type qualifier unless the C spec absolutely demands it.

Ok. Thanks for the information. I was mislead by
comment in clang::Qualifiers::TQ
// NOTE: These flags must be kept in sync with DeclSpec::TQ.
and the test where 5 bits value was written into 3 bits field.

Would be great if "note" can be clarified as well.

Thanks
Vladimir.

5

Ping,

Could someone comment, please, if clang::Qualifiers::TQ intentionally reduces number of flags from 5 to 3?

It is not intentional, no. We do not represent _Atomic as an ordinary qualifier in the AST, so I think this might accidentally be okay there. Offhand, I don't think we want to support either of those qualifiers as an array index type qualifier unless the C spec absolutely demands it.

Ok. Thanks for the information. I was mislead by
comment in clang::Qualifiers::TQ
// NOTE: These flags must be kept in sync with DeclSpec::TQ.
and the test where 5 bits value was written into 3 bits field.

Would be great if "note" can be clarified as well.

If something is just blindly trying to propagate the DeclSpec::TQ around, that seems problematic.

I think the comment is more about the flag values than the full set of flags, but yes, that should be clarified.

John.

6

Ping,

Could someone comment, please, if clang::Qualifiers::TQ intentionally reduces number of flags from 5 to 3?

It is not intentional, no. We do not represent _Atomic as an ordinary qualifier in the AST, so I think this might accidentally be okay there. Offhand, I don't think we want to support either of those qualifiers as an array index type qualifier unless the C spec absolutely demands it.

Ok. Thanks for the information. I was mislead by
comment in clang::Qualifiers::TQ
// NOTE: These flags must be kept in sync with DeclSpec::TQ.
and the test where 5 bits value was written into 3 bits field.

Would be great if "note" can be clarified as well.

If something is just blindly trying to propagate the DeclSpec::TQ around, that seems problematic.

If only 3 flags are really wanted here, then probably this line

ArrayTypeBits.IndexTypeQuals = tq;

should use cleaning mask:
ArrayTypeBits.IndexTypeQuals = tq & Qualifiers::CVRMask;

Vladimir.

7

Ping,

Could someone comment, please, if clang::Qualifiers::TQ intentionally reduces number of flags from 5 to 3?

It is not intentional, no. We do not represent _Atomic as an ordinary qualifier in the AST, so I think this might accidentally be okay there. Offhand, I don't think we want to support either of those qualifiers as an array index type qualifier unless the C spec absolutely demands it.

Ok. Thanks for the information. I was mislead by
comment in clang::Qualifiers::TQ
// NOTE: These flags must be kept in sync with DeclSpec::TQ.
and the test where 5 bits value was written into 3 bits field.

Would be great if "note" can be clarified as well.

If something is just blindly trying to propagate the DeclSpec::TQ around, that seems problematic.

If only 3 flags are really wanted here, then probably this line

ArrayTypeBits.IndexTypeQuals = tq;

should use cleaning mask:
ArrayTypeBits.IndexTypeQuals = tq & Qualifiers::CVRMask;

I think that would be an excellent clarification in the code, yeah.

John.