VLA detection ...

VLA detection ...

[Willem Jan Withagen]

Hi,

Now that we have -Wvla set:
===
    Adds C++ warning flag for C Variable-Length Arrays.

    C VLAs are not supported in C++. However, the GNU compiler allows
    them as an extension, which it does not warn about when not in
    pedantic mode. Unfortunately, it's easy to accidentally write
    a VLA, even unintentionally-- adding this warning will help us
    catch that.
===

Are there going to be attempts to eradicate this usage?
Note that Clang also supports VLAs:
===
GCC and C99 allow an array's size to be determined at run time. This
extension is not permitted in standard C++. However, Clang supports such
variable length arrays for compatibility with GNU C and C99 programs.
===

--WjW

Re: VLA detection ...

[kefu chai]

On Sat, Jun 3, 2017 at 2:50 AM, Willem Jan Withagen <wjw@digiware.nl> wrote:
> Hi,
>
> Now that we have -Wvla set:
> ===
>     Adds C++ warning flag for C Variable-Length Arrays.
>
>     C VLAs are not supported in C++. However, the GNU compiler allows
>     them as an extension, which it does not warn about when not in
>     pedantic mode. Unfortunately, it's easy to accidentally write
>     a VLA, even unintentionally-- adding this warning will help us
>     catch that.
> ===
>
> Are there going to be attempts to eradicate this usage?
> Note that Clang also supports VLAs:
> ===
> GCC and C99 allow an array's size to be determined at run time. This
> extension is not permitted in standard C++. However, Clang supports such
> variable length arrays for compatibility with GNU C and C99 programs.
> ===
>

it's not in C++11 standard. but it's a handy feature in practice. i
still have trouble understanding we want to have -Wvla. i was trying
to silence this warning option last night but failed to move further
after several attempts. see https://github.com/ceph/ceph/pull/15441.

probably, Jesse have more insights on this.



> --WjW
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Regards
Kefu Chai

Re: VLA detection ...

[Willem Jan Withagen]

On 04/06/2017 05:28, kefu chai wrote:
> On Sat, Jun 3, 2017 at 2:50 AM, Willem Jan Withagen <wjw@digiware.nl> wrote:
>> Hi,
>>
>> Now that we have -Wvla set:
>> ===
>>     Adds C++ warning flag for C Variable-Length Arrays.
>>
>>     C VLAs are not supported in C++. However, the GNU compiler allows
>>     them as an extension, which it does not warn about when not in
>>     pedantic mode. Unfortunately, it's easy to accidentally write
>>     a VLA, even unintentionally-- adding this warning will help us
>>     catch that.
>> ===
>>
>> Are there going to be attempts to eradicate this usage?
>> Note that Clang also supports VLAs:
>> ===
>> GCC and C99 allow an array's size to be determined at run time. This
>> extension is not permitted in standard C++. However, Clang supports such
>> variable length arrays for compatibility with GNU C and C99 programs.
>> ===
>>
> 
> it's not in C++11 standard. but it's a handy feature in practice. i
> still have trouble understanding we want to have -Wvla. i was trying
> to silence this warning option last night but failed to move further
> after several attempts. see https://github.com/ceph/ceph/pull/15441.
> 
> probably, Jesse have more insights on this.

It starts with old discussions from before 2009, and it would be hard
for compiler makers to actually build it.

Both GCC and Clang support it.

And next the major complaint I find about it, is that these structures
are build on the stack, which might cause stackoverlows (and that mostly
on embedded computers) That would be something for programmers to be
aware of.

And C++ has other stuctures to implement these.
But as you tried, it is nog going to be easy.

I would probably think there are beter things to do, than to chase after
this. And IMHO the order should have been: Fix the VLAs and only then
turn the warning on on the master repo. As to not have every body start
looking at these warning that have just started to pop up. But perhaps
that is just me, because each and every warning is again a case where I
need to look for possible incompatabilities.


--WjW

Re: VLA detection ...

[kefu chai]

On Sun, Jun 4, 2017 at 4:29 PM, Willem Jan Withagen <wjw@digiware.nl> wrote:
> On 04/06/2017 05:28, kefu chai wrote:
>> On Sat, Jun 3, 2017 at 2:50 AM, Willem Jan Withagen <wjw@digiware.nl> wrote:
>>> Hi,
>>>
>>> Now that we have -Wvla set:
>>> ===
>>>     Adds C++ warning flag for C Variable-Length Arrays.
>>>
>>>     C VLAs are not supported in C++. However, the GNU compiler allows
>>>     them as an extension, which it does not warn about when not in
>>>     pedantic mode. Unfortunately, it's easy to accidentally write
>>>     a VLA, even unintentionally-- adding this warning will help us
>>>     catch that.
>>> ===
>>>
>>> Are there going to be attempts to eradicate this usage?
>>> Note that Clang also supports VLAs:
>>> ===
>>> GCC and C99 allow an array's size to be determined at run time. This
>>> extension is not permitted in standard C++. However, Clang supports such
>>> variable length arrays for compatibility with GNU C and C99 programs.
>>> ===
>>>
>>
>> it's not in C++11 standard. but it's a handy feature in practice. i
>> still have trouble understanding we want to have -Wvla. i was trying
>> to silence this warning option last night but failed to move further
>> after several attempts. see https://github.com/ceph/ceph/pull/15441.
>>
>> probably, Jesse have more insights on this.
>
> It starts with old discussions from before 2009, and it would be hard
> for compiler makers to actually build it.

i am confused. as long as GCC and Clang claim to support a feature, we
should be able to it unless we have other concerns like portability or
performance. what we should be concerned is build-time and run-time
performance of ceph, not how hard this was implemented by GCC or Clang
folks, right?

>
> Both GCC and Clang support it.
>
> And next the major complaint I find about it, is that these structures
> are build on the stack, which might cause stackoverlows (and that mostly
> on embedded computers) That would be something for programmers to be
> aware of.

it does. but i think this is why i like it. it avoid the overhead of
heap allocation. agreed. but if it's a bug, then we need to fix it
instead of using a slower replacement. for example, if we allocate a
buffer of 25MB on stack, we should probably just crash. the existing
std::vector or boost::container::small_vector does not do this for us.

>
> And C++ has other stuctures to implement these.
> But as you tried, it is nog going to be easy.
>
> I would probably think there are beter things to do, than to chase after
> this. And IMHO the order should have been: Fix the VLAs and only then
> turn the warning on on the master repo. As to not have every body start

again, agreed.

> looking at these warning that have just started to pop up. But perhaps
> that is just me, because each and every warning is again a case where I
> need to look for possible incompatabilities.

these warnings trouble me also.

>
>
> --WjW



-- 
Regards
Kefu Chai

Re: VLA detection ...

[Sage Weil]

On Sun, 4 Jun 2017, Willem Jan Withagen wrote:
> On 04/06/2017 05:28, kefu chai wrote:
> > On Sat, Jun 3, 2017 at 2:50 AM, Willem Jan Withagen <wjw@digiware.nl> wrote:
> >> Hi,
> >>
> >> Now that we have -Wvla set:
> >> ===
> >>     Adds C++ warning flag for C Variable-Length Arrays.
> >>
> >>     C VLAs are not supported in C++. However, the GNU compiler allows
> >>     them as an extension, which it does not warn about when not in
> >>     pedantic mode. Unfortunately, it's easy to accidentally write
> >>     a VLA, even unintentionally-- adding this warning will help us
> >>     catch that.
> >> ===
> >>
> >> Are there going to be attempts to eradicate this usage?
> >> Note that Clang also supports VLAs:
> >> ===
> >> GCC and C99 allow an array's size to be determined at run time. This
> >> extension is not permitted in standard C++. However, Clang supports such
> >> variable length arrays for compatibility with GNU C and C99 programs.
> >> ===
> >>
> > 
> > it's not in C++11 standard. but it's a handy feature in practice. i
> > still have trouble understanding we want to have -Wvla. i was trying
> > to silence this warning option last night but failed to move further
> > after several attempts. see https://github.com/ceph/ceph/pull/15441.
> > 
> > probably, Jesse have more insights on this.
> 
> It starts with old discussions from before 2009, and it would be hard
> for compiler makers to actually build it.
> 
> Both GCC and Clang support it.
> 
> And next the major complaint I find about it, is that these structures
> are build on the stack, which might cause stackoverlows (and that mostly
> on embedded computers) That would be something for programmers to be
> aware of.

I'd consider this a feature more than a risk.  It means we can 
allocate a modest-sized array without hitting the allocator.  My vote 
is to just revert -Wvla...

> I would probably think there are beter things to do, than to chase after
> this. And IMHO the order should have been: Fix the VLAs and only then
> turn the warning on on the master repo. As to not have every body start
> looking at these warning that have just started to pop up. But perhaps
> that is just me, because each and every warning is again a case where I
> need to look for possible incompatabilities.

Yep, my bad for assuming the cleanup was already done (or trivial)!

sage

Re: VLA detection ...

[Willem Jan Withagen]

On 04/06/2017 20:28, Sage Weil wrote:
> On Sun, 4 Jun 2017, Willem Jan Withagen wrote:
>> On 04/06/2017 05:28, kefu chai wrote:
>>> On Sat, Jun 3, 2017 at 2:50 AM, Willem Jan Withagen <wjw@digiware.nl> wrote:
>>>> Hi,
>>>>
>>>> Now that we have -Wvla set:
>>>> ===
>>>>     Adds C++ warning flag for C Variable-Length Arrays.
>>>>
>>>>     C VLAs are not supported in C++. However, the GNU compiler allows
>>>>     them as an extension, which it does not warn about when not in
>>>>     pedantic mode. Unfortunately, it's easy to accidentally write
>>>>     a VLA, even unintentionally-- adding this warning will help us
>>>>     catch that.
>>>> ===
>>>>
>>>> Are there going to be attempts to eradicate this usage?
>>>> Note that Clang also supports VLAs:
>>>> ===
>>>> GCC and C99 allow an array's size to be determined at run time. This
>>>> extension is not permitted in standard C++. However, Clang supports such
>>>> variable length arrays for compatibility with GNU C and C99 programs.
>>>> ===
>>>>
>>>
>>> it's not in C++11 standard. but it's a handy feature in practice. i
>>> still have trouble understanding we want to have -Wvla. i was trying
>>> to silence this warning option last night but failed to move further
>>> after several attempts. see https://github.com/ceph/ceph/pull/15441.
>>>
>>> probably, Jesse have more insights on this.
>>
>> It starts with old discussions from before 2009, and it would be hard
>> for compiler makers to actually build it.
>>
>> Both GCC and Clang support it.
>>
>> And next the major complaint I find about it, is that these structures
>> are build on the stack, which might cause stackoverlows (and that mostly
>> on embedded computers) That would be something for programmers to be
>> aware of.
> 
> I'd consider this a feature more than a risk.  It means we can 
> allocate a modest-sized array without hitting the allocator.  My vote 
> is to just revert -Wvla...
> 
>> I would probably think there are beter things to do, than to chase after
>> this. And IMHO the order should have been: Fix the VLAs and only then
>> turn the warning on on the master repo. As to not have every body start
>> looking at these warning that have just started to pop up. But perhaps
>> that is just me, because each and every warning is again a case where I
>> need to look for possible incompatabilities.
> 
> Yep, my bad for assuming the cleanup was already done (or trivial)!

Took the liberty to submit a revert:
https://github.com/ceph/ceph/pull/15469

--WjW

Re: VLA detection ...

[Jesse Williamson]

On Sun, 4 Jun 2017, Willem Jan Withagen wrote:

Hi,

>>> And next the major complaint I find about it, is that these structures
>>> are build on the stack, which might cause stackoverlows (and that mostly
>>> on embedded computers) That would be something for programmers to be
>>> aware of.

There are a few potential issues to consider. I'll highlight a couple of 
them.

I think it's definitely important to mention that VLAs are certainly 
useful, and there are cases where they can be important for performance 
reasons. This is why I suggest -Wvla rather than -Wvla-error (though I'm 
okay with that, too, and disabling on a case-by-case basis).

The main issue with VLAs: they're really, really easy to write.

While this is in part a feature, it also means that programmers can use 
this feature entirely inadvertently. Seeing this happen "in the wild" is 
actually why I'm suggesting adding -Wvla: they can be useful, but I think 
the potential issues they can cause, and resultant effort debugging, make 
them worth warning about since they're so easy to invoke by accident.

VLAs allow unbounded and entirely unchecked allocation on the stack. There 
is no way to detect overflow or any other error. If the allocation exceeds 
the available stack space, the call stack is straight-up clobbered, which 
is not very easy to debug when it's the third item in a 30 layer deep call 
stack that's spoiling the fun. Note that some platforms start the stack at 
the end of the address space and count backwards.

Maybe a miscalculated VLA will crash. But if you're having a bad Monday, 
that won't happen: the program might keep ticking along for hours, with 
random parts of memory silently corrupted. (Note: alloca() does not save 
you from this.) There's no way to detect this error.

And, this can also go in the other direction-- consider what happens here:
 	int foo[bar] = { 1, 2, 3, 4, 5, 6 };
...great, unless bar is < 6... but you won't know until things go wrong.

Finally, note that sizeof(my_vla) is a /runtime/ calculation.

Anyway, as I said, I'm not suggesting we outright ban them, but I think a 
warning is a good idea because I've seen people write these completely 
accidentally. There's a lot of current effort in the committee to provide 
a feature like this, but to my knowledge it's not yet settled-- other than 
general agreement that the C approach doesn't work so well in C++.[1]

Naturally, we're used to courting some danger since we're using C family 
languages, but I hope I've helped to at least raise awareness as to why it 
may be a good idea to warn when this non-standard feature is used.

* Some more background on VLAs and C++:
         http://www.stroustrup.com/examples_short.pdf

 	There have been some proposals to add features like this, but no takers
 	yet. There are others, but for example (and some discussion):
 	http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3497.html
 	http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3810.pdf

>> Yep, my bad for assuming the cleanup was already done (or trivial)!

My bad for not submitting patches along with adding -Wvla, and not 
flagging the PR "RFC", which is what I should have done. Sorry for the 
trouble.

-Jesse

1. This is a discussion that seems to generate some heat:
"https://groups.google.com/forum/#!searchin/comp.std.c$2B$2B/vlas|sort:relevance/comp.std.c++/K_4lgA1JYeg/2qRcUvmGhBEJ"

Re: VLA detection ...

[Willem Jan Withagen]

On 05/06/2017 22:02, Jesse Williamson wrote:
> On Sun, 4 Jun 2017, Willem Jan Withagen wrote:
> 
> Hi,
> 
>>>> And next the major complaint I find about it, is that these structures
>>>> are build on the stack, which might cause stackoverlows (and that
>>>> mostly
>>>> on embedded computers) That would be something for programmers to be
>>>> aware of.
> 
> There are a few potential issues to consider. I'll highlight a couple of
> them.
> 
> I think it's definitely important to mention that VLAs are certainly
> useful, and there are cases where they can be important for performance
> reasons. This is why I suggest -Wvla rather than -Wvla-error (though I'm
> okay with that, too, and disabling on a case-by-case basis).
> 
> The main issue with VLAs: they're really, really easy to write.
> 
> While this is in part a feature, it also means that programmers can use
> this feature entirely inadvertently. Seeing this happen "in the wild" is
> actually why I'm suggesting adding -Wvla: they can be useful, but I
> think the potential issues they can cause, and resultant effort
> debugging, make them worth warning about since they're so easy to invoke
> by accident.
> 
> VLAs allow unbounded and entirely unchecked allocation on the stack.
> There is no way to detect overflow or any other error. If the allocation
> exceeds the available stack space, the call stack is straight-up
> clobbered, which is not very easy to debug when it's the third item in a
> 30 layer deep call stack that's spoiling the fun. Note that some
> platforms start the stack at the end of the address space and count
> backwards.
> 
> Maybe a miscalculated VLA will crash. But if you're having a bad Monday,
> that won't happen: the program might keep ticking along for hours, with
> random parts of memory silently corrupted. (Note: alloca() does not save
> you from this.) There's no way to detect this error.
> 
> And, this can also go in the other direction-- consider what happens here:
>     int foo[bar] = { 1, 2, 3, 4, 5, 6 };
> ...great, unless bar is < 6... but you won't know until things go wrong.
> 
> Finally, note that sizeof(my_vla) is a /runtime/ calculation.

Excuse me for sounding like an old fart, but I've been programming way
too long already. Yes I do like programming in Pascal and Ada, fully
structured and you cannot even get going without putting your seatbelts
on first.

I think your remarks are all valid and referencing VLA could/should be
done with some careful gards. On the otherhand the code is full with
pointer, and not all constructs are 100% C++ type, and can also run out
of bound. (As I've already noticed a few times)

So care has to be taken when VLA are used, as with all constructs one
selects to solve a problem.

> Anyway, as I said, I'm not suggesting we outright ban them, but I think
> a warning is a good idea because I've seen people write these completely
> accidentally. There's a lot of current effort in the committee to
> provide a feature like this, but to my knowledge it's not yet settled--
> other than general agreement that the C approach doesn't work so well in
> C++.[1]

The disadvantage with just switching on warnings is that they start to
bloat. And as such start to create noise, masquerading/distracting from
new real important warning. Because that is what is going to happen:
People start just reading over them, and then some more.

> Naturally, we're used to courting some danger since we're using C family
> languages, but I hope I've helped to at least raise awareness as to why
> it may be a good idea to warn when this non-standard feature is used.

I, for one, disagree with that. It is not going to help the problem by
switching the warnings on. The actual issue needs to be fixed, Kefu
tried that, and declared his first attempt not really satisfactory.

So I think this is going to stay here until "the stds group", in its
infinite wisdom has designed a solution for this that actually works.
Until then, all bets are of.

> * Some more background on VLAs and C++:
>         http://www.stroustrup.com/examples_short.pdf
> 
>     There have been some proposals to add features like this, but no takers
>     yet. There are others, but for example (and some discussion):
>     http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3497.html
>     http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3810.pdf

Not to sound pedantic, but this is all part of this of code requirements
that are sort underlying this project. But I have yet to see that people
are being hit over the head with these rules. (Unlike in other projects
where I work and we had regular audits to check confirmity to the code
rules)

>>> Yep, my bad for assuming the cleanup was already done (or trivial)!
> 
> My bad for not submitting patches along with adding -Wvla, and not
> flagging the PR "RFC", which is what I should have done. Sorry for the
> trouble.

No harm there, it is of for the time being. And if you feel really
strong about this, feel free to run this on your private repo and start
fixing the VLAs. I think everybody will welcome you submissions.
Or be like me, I try to keep things FreeBSD compatible, and try to chase
people using VLAs suggesting that they pick a different solution.

Lets get back to programming.....

--WjW

> -Jesse
> 
> 1. This is a discussion that seems to generate some heat:
> "https://groups.google.com/forum/#!searchin/comp.std.c$2B$2B/vlas|sort:relevance/comp.std.c++/K_4lgA1JYeg/2qRcUvmGhBEJ"
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: VLA detection ...

[Jesse Williamson]

On Mon, 5 Jun 2017, Willem Jan Withagen wrote:

Hi Willem,

Thank you for your comments!

> I think your remarks are all valid and referencing VLA could/should be
> done with some careful gards. On the otherhand the code is full with
> pointer, and not all constructs are 100% C++ type, and can also run out
> of bound. (As I've already noticed a few times)

We certainly need to be able to use "dangerous" constructs, and to be sure 
they're present for good reasons.

In my view, the problem isn't that VLAs exist, it's that're so easy to use 
by complete accident, and if ever mis-used can lead to some extremely 
subtle problems that in a bad case are difficult to reproduce and even 
harder to find.

> So care has to be taken when VLA are used, as with all constructs one
> selects to solve a problem.

Definitely agreed.

> The disadvantage with just switching on warnings is that they start to
> bloat. And as such start to create noise, masquerading/distracting from
> new real important warning. Because that is what is going to happen:
> People start just reading over them, and then some more.

I'm happy to provide patches, and resubmit the PR at some point soon. 
Completely agreed that warnings have to be useful, and if we ignore them 
then they might as well not be there.

> I, for one, disagree with that. It is not going to help the problem by
> switching the warnings on. The actual issue needs to be fixed, Kefu
> tried that, and declared his first attempt not really satisfactory.

I'll try to catch up with Kefu and see where the pain points were.

> So I think this is going to stay here until "the stds group", in its
> infinite wisdom has designed a solution for this that actually works.
> Until then, all bets are of.

I know that there are some relevant proposals, but AFAIK not super strong 
traction just yet.

As long as nothing's actually exploded, I suppose it's like pointing at a 
bunch of TNT and saying "hey, we probably shouldn't smoke near that", but 
nobody's happend to do it yet so it seems fine. ;-)

> Not to sound pedantic, but this is all part of this of code requirements
> that are sort underlying this project. But I have yet to see that people
> are being hit over the head with these rules. (Unlike in other projects
> where I work and we had regular audits to check confirmity to the code
> rules)

It's unclear to me what extensions we do and do not allow. If the project 
is in standard C++11, VLAs shouldn't be allowed because they're not part 
of the language. Then again, that sort of pedanticism may be of limited 
actual utility.

> Or be like me, I try to keep things FreeBSD compatible, and try to chase
> people using VLAs suggesting that they pick a different solution.

I'll try to see where we're using them now and get a sense of where they 
are specifically chosen and useful.

> Lets get back to programming.....

Always a good plan! :-)

-Jesse