r348276 - [AST][NFC] Make ArrayTypeTraitExpr non polymorphic

[Mikael Holmén via cfe-commits]

On 12/12/18 2:17 PM, Bruno Ricci wrote:
> 
> 
> On 12/12/2018 07:27, Mikael Holmén wrote:
>> Hi,
>>
>> On 12/11/18 5:49 PM, Bruno Ricci wrote:
>>> Hi Mikael,
>>>
>>> I can indeed reproduce this with gcc 5.5.0 when doing a Release
>>> build with assertions. I cannot reproduce this with gcc 6.5.0
>>> (also with a Release build with assertions), nor can I reproduce
>>> this with clang 7 (also with a Release build with assertions).
>>>
>>> I tried to instrument StringRefCheckerVisitor::VisitChildren but this
>>> just causes the stack overflow to move to CGBuilder.
>>>
>>> What is stranger is that the reproducer do not even causes
>>> an ArrayTypeTraitExpr node to be created.
>>>
>>> Increasing the maximum stack size with ulimit by a small amount fixes
>>> the problem, so I don't think this is a mis-compilation either.
>>
>> Oh, I didn't realize it actually passed with a larger stack, I thought
>> it recursed forever.
>>
>> I see now that before the change clang-tidy compiled with gcc 5.4 passed
>> with stacksize limited to 2288k and after it needs 9372k.
>>
>> If I use clang 3.6 to compile I needed 1747k before the change and 1750k
>> after.
> 
> Huh ? This is a huge increase.
> 

Yes, well compared to the insane increase for gcc 5.4 it's nothing :)

But perhaps the increase with clang is interesting as well, I don't know.

>>
>>>
>>> I think that this change is correct since no one is using the polymorphism
>>> of ArrayTypeTraitExpr. Indeed taking the blunt step of making it final do
>>> not causes any failure.
>>>
>>> The only possible explaination I can think of is that for one reason
>>> or another, with this particular compiler + build settings, this change
>>> causes an increase of stack usage which exceeed the default limit ?
>>>
>>
>> Yes perhaps it's just some deficiency in old gcc versions that is
>> exposed by this then.
>>
>> Thanks,
>> Mikael
> 
> Perhaps it is just indeed a deficiency in gcc 5.4, but this is strange.
> 

Yes. I'm not sure what to do with this, apart from that we will speed up 
our team's transition from gcc 5.4 to something newer. That would be 
needed anyway due to the fact that llvm wants to start using C++17, but 
we'll try to do that sooner rather than later now.

Regards,
Mikael

> Bruno
>>
>>
>>> Regards,
>>> Bruno