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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sumner, Brian via cfe-dev
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.



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sumner, Brian via cfe-dev
Ping,

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

Thanks,
Vladimir.

On 07.08.2017 16:06, Vladimir Voskresensky via cfe-dev wrote:

> 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.
>
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sumner, Brian via cfe-dev
> On Aug 9, 2017, at 4:51 PM, Vladimir Voskresensky via cfe-dev <[hidden email]> wrote:
> 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.

>
> Thanks,
> Vladimir.
>
> On 07.08.2017 16:06, Vladimir Voskresensky via cfe-dev wrote:
>> 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.
>>
>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sumner, Brian via cfe-dev


On 10.08.2017 02:31, John McCall wrote:
>> On Aug 9, 2017, at 4:51 PM, Vladimir Voskresensky via cfe-dev <[hidden email]> wrote:
>> 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.

>
> John.
>
>> Thanks,
>> Vladimir.
>>
>> On 07.08.2017 16:06, Vladimir Voskresensky via cfe-dev wrote:
>>> 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.
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sumner, Brian via cfe-dev
> On Aug 10, 2017, at 7:56 AM, Vladimir Voskresensky <[hidden email]> wrote:
> On 10.08.2017 02:31, John McCall wrote:
>>> On Aug 9, 2017, at 4:51 PM, Vladimir Voskresensky via cfe-dev <[hidden email]> wrote:
>>> 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.

>
> Thanks
> Vladimir.
>
>>
>> John.
>>
>>> Thanks,
>>> Vladimir.
>>>
>>> On 07.08.2017 16:06, Vladimir Voskresensky via cfe-dev wrote:
>>>> 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.
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> [hidden email]
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sumner, Brian via cfe-dev


On 10.08.2017 18:19, John McCall wrote:

>> On Aug 10, 2017, at 7:56 AM, Vladimir Voskresensky <[hidden email]> wrote:
>> On 10.08.2017 02:31, John McCall wrote:
>>>> On Aug 9, 2017, at 4:51 PM, Vladimir Voskresensky via cfe-dev <[hidden email]> wrote:
>>>> 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.

>
> I think the comment is more about the flag values than the full set of flags, but yes, that should be clarified.
>
> John.
>
>> Thanks
>> Vladimir.
>>
>>> John.
>>>
>>>> Thanks,
>>>> Vladimir.
>>>>
>>>> On 07.08.2017 16:06, Vladimir Voskresensky via cfe-dev wrote:
>>>>> 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.
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-dev mailing list
>>>>> [hidden email]
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> [hidden email]
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sumner, Brian via cfe-dev
> On Aug 10, 2017, at 11:31 AM, Vladimir Voskresensky <[hidden email]> wrote:
> On 10.08.2017 18:19, John McCall wrote:
>>> On Aug 10, 2017, at 7:56 AM, Vladimir Voskresensky <[hidden email]> wrote:
>>> On 10.08.2017 02:31, John McCall wrote:
>>>>> On Aug 9, 2017, at 4:51 PM, Vladimir Voskresensky via cfe-dev <[hidden email]> wrote:
>>>>> 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.

>
> Vladimir.
>>
>> I think the comment is more about the flag values than the full set of flags, but yes, that should be clarified.
>>
>> John.
>>
>>> Thanks
>>> Vladimir.
>>>
>>>> John.
>>>>
>>>>> Thanks,
>>>>> Vladimir.
>>>>>
>>>>> On 07.08.2017 16:06, Vladimir Voskresensky via cfe-dev wrote:
>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-dev mailing list
>>>>>> [hidden email]
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>> _______________________________________________
>>>>> cfe-dev mailing list
>>>>> [hidden email]
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Loading...