[PATCH] RE: [cfe-dev] missing return statement for non-void functions in C++

Sjoerd Meijer via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 14 09:14:23 PDT 2015


Yes, I got confused when clang is in C++ mode. In my patch, I was checking the driver status, but that’s not the complete story. From these combinations of invoking clang with some input:

./clang[++] [-x [c | c++]] myinput.[TY_C | TY_CXX]

I wanted to create a special case for these combinations only:

./clang++ ret.c
./clang -x c++ ret.c
(./clang++ -x c++ ret.c)

Which are the cases when clang is in C++ mode and the file extension is C. For these combinations, I wanted to allow this new –fallow-nonreturning-functions flag (ignore it for the others cases), thus generate function epilogues which came from a request to compile legacy C code with a C++ compiler. I’ve now modified my patch, and I think it was as simple as checking, in pseudo code, “types::isCXX() && LookupTypeForExtension() == types::TY_C). However, I won’t submit the patch yet unless you think it is still useful; I will also see if we can solve this compilation of legacy code in a different way.

Regardless of this discussion and new option, not returning from a function is not very nice. Probably the best thing is not only generate a trap at optimisation level O0, but in all cases? That’s a quick and easy fix. Probably the security folks can live with this as well as the folks who want to benefit from this UB as an optimisation opportunity.

Cheers,
Sjoerd.


From: metafoo at gmail.com [mailto:metafoo at gmail.com] On Behalf Of Richard Smith
Sent: 13 August 2015 19:18
To: Sjoerd Meijer
Cc: Hal Finkel; Marshall Clow; cfe-commits
Subject: Re: [PATCH] RE: [cfe-dev] missing return statement for non-void functions in C++

On Thu, Aug 13, 2015 at 1:52 AM, Sjoerd Meijer <Sjoerd.Meijer at arm.com<mailto:Sjoerd.Meijer at arm.com>> wrote:
Hi Richard,
Thanks for reviewing. Agree, that was a bit confusing. More specifically,
the warning message was confusing (i.e. wrong). This patch is for compiling .c
input in C++ mode. The new flag should be ignored for C++ *input*, and indeed
not when it is in C++ *mode* as the warning message said earlier. So I have
changed the warning message accordingly and hope that solves it, see attached
patch.

What is the distinction you're trying to draw here? This patch still doesn't make sense to me. This flag is only meaningful when compiling as C++. You ignore it when compiling as C but produce a warning that says it's ignored when compiling as C++.

Cheers.

From: metafoo at gmail.com<mailto:metafoo at gmail.com> [mailto:metafoo at gmail.com<mailto:metafoo at gmail.com>] On Behalf Of Richard Smith
Sent: 12 August 2015 23:06
To: Sjoerd Meijer
Cc: Hal Finkel; Marshall Clow; cfe-commits

Subject: Re: [PATCH] RE: [cfe-dev] missing return statement for non-void functions in C++

This patch seems a bit confused. You warn that the flag is ignored in C++, but it only has an effect in C++. You have a testcase with a .c extension that is built with -x c++.

On Wed, Aug 12, 2015 at 5:23 AM, Sjoerd Meijer <sjoerd.meijer at arm.com<mailto:sjoerd.meijer at arm.com>> wrote:
[ + cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org> ]

Hi,
The functionality is now available under a flag, see attached patch. Note that the flag is ignored in C++ mode, so it will help the use case of compiling (legacy) C code with a C++ compiler.
Cheers,
Sjoerd.

From: Sjoerd Meijer [mailto:sjoerd.meijer at arm.com]
Sent: 03 August 2015 11:40
To: 'Richard Smith'
Cc: Hal Finkel; Marshall Clow; cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu> Developers; cfe commits
Subject: RE: [PATCH] RE: [cfe-dev] missing return statement for non-void functions in C++

Hi Richard,

I agree with your conclusions and will start preparing a patch for option 3) under a flag that is off by default; this enables folks to build/run C code in C++. I actually think option 2) would be a good one too, but as it is already available under a flag I also don’t see how useful it is combining options 2) and 3) with another (or one more) flag that is off by default.

Cheers.

From: metafoo at gmail.com<mailto:metafoo at gmail.com> [mailto:metafoo at gmail.com] On Behalf Of Richard Smith
Sent: 31 July 2015 19:46
To: Sjoerd Meijer
Cc: Hal Finkel; Marshall Clow; cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu> Developers; cfe commits
Subject: Re: [PATCH] RE: [cfe-dev] missing return statement for non-void functions in C++

On Fri, Jul 31, 2015 at 7:35 AM, Sjoerd Meijer <sjoerd.meijer at arm.com<mailto:sjoerd.meijer at arm.com>> wrote:
Hi, I am not sure if we came to a conclusion. Please find attached a patch. It simply removes the two lines that insert an unreachable statement (which cause removal of the return statement). Please note that at -O0 the trap instruction is still generated. Is this something we could live with?

I don't think this is an improvement:

This doesn't satisfy the folks who want an 'unreachable' for better code size and optimization, and it doesn't satisfy the folks who want a guaranteed trap for security, and it doesn't satisfy the folks who want their broken code to limp along (because it'll still trap at -O0), and it is at best a minor improvement for the folks who want missing returns to be more easily debuggable (with -On, the code goes wrong in the caller, or appears to work, rather than falling into an unrelated function, and debugging this with -O0 was already easy).

I think there are three options that are defensible here:
1) The status quo: this is UB and we treat it as such and optimize on that basis, but provide a trap as a convenience at -O0
2) The secure approach: this is UB but we always trap
3) Define the behavior to return 'undef' for C types: this allows questionable C code that has UB in C++ to keep working when built with a C++ compiler

Note that (3) can be combined with either (1) or (2). (2) is already available via the 'return' sanitizer. So this really reduces to: in those cases where C says it's OK so long as the caller doesn't look at the returned value (and where the return type doesn't have a non-trivial copy constructor or destructor, isn't a reference, and so on), should we attempt to preserve the C behaviour? I would be OK with putting that behind a `-f` flag (perhaps `-fstrict-return` or similar) to support those folks who want to build C code in C++, but I would suggest having that flag be off by default, since that is not the usual use case for a C++ compiler.

Cheers,
Sjoerd.

From: cfe-dev-bounces at cs.uiuc.edu<mailto:cfe-dev-bounces at cs.uiuc.edu> [mailto:cfe-dev-bounces at cs.uiuc.edu<mailto:cfe-dev-bounces at cs.uiuc.edu>] On Behalf Of Richard Smith
Sent: 29 July 2015 18:07
To: Hal Finkel
Cc: Marshall Clow; cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu> Developers

Subject: Re: [cfe-dev] missing return statement for non-void functions in C++


On Jul 29, 2015 7:43 AM, "Hal Finkel" <hfinkel at anl.gov<mailto:hfinkel at anl.gov>> wrote:
>
> ----- Original Message -----
> > From: "David Blaikie" <dblaikie at gmail.com<mailto:dblaikie at gmail.com>>
> > To: "James Molloy" <james at jamesmolloy.co.uk<mailto:james at jamesmolloy.co.uk>>
> > Cc: "Marshall Clow" <mclow.lists at gmail.com<mailto:mclow.lists at gmail.com>>, "cfe-dev Developers" <cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu>>
> > Sent: Wednesday, July 29, 2015 9:15:09 AM
> > Subject: Re: [cfe-dev] missing return statement for non-void functions in C++
> >
> >
> > On Jul 29, 2015 7:06 AM, "James Molloy" < james at jamesmolloy.co.uk<mailto:james at jamesmolloy.co.uk> >
> > wrote:
> > >
> > > Hi,
> > >
> > > If we're going to emit a trap instruction (and thus create a broken
> > > binary), why don't we error instead?
> >
> > We warn, can't error, because it may be dynamically unreached, in
> > which case the program is valid and we can't reject it.
>
> I think this also explains why this is useful for optimization.
>
>  1. It is a code-size optimization
>  2. By eliminating unreachable control flow, we can remove branches and tests that are not actual necessary
>
> int foo(int x) {
>   if (x > 5) return 2*x;
>   else if (x < 2) return 3 - x;
> }
>
> That having been said, there are other ways to express these things, and the situation often represents an error. I'd be fine with requiring a special flag (-fallow-nonreturning-functions or whatever) in order to put the compiler is a truly confirming mode (similar to the situation with sized delete).

Note that we already have a flag to trap on this: -fsanitize-trap=return. (You may also need -fsanitize=return, I don't remember.) That seems consistent with how we treat most other forms of UB.

>  -Hal
>
> >
> > >
> > > James
> > >
> > > On Wed, 29 Jul 2015 at 15:05 David Blaikie < dblaikie at gmail.com<mailto:dblaikie at gmail.com> >
> > > wrote:
> > >>
> > >>
> > >> On Jul 29, 2015 2:10 AM, "mats petersson" < mats at planetcatfish.com<mailto:mats at planetcatfish.com>
> > >> > wrote:
> > >> >
> > >> >
> > >> >
> > >> > On 28 July 2015 at 23:40, Marshall Clow < mclow.lists at gmail.com<mailto:mclow.lists at gmail.com>
> > >> > > wrote:
> > >> >>
> > >> >>
> > >> >>
> > >> >> On Tue, Jul 28, 2015 at 6:14 AM, Sjoerd Meijer <
> > >> >> sjoerd.meijer at arm.com<mailto:sjoerd.meijer at arm.com> > wrote:
> > >> >>>
> > >> >>> Hi,
> > >> >>>
> > >> >>>
> > >> >>>
> > >> >>> In C++, the undefined behaviour of a missing return statements
> > >> >>> for a non-void function results in not generating the
> > >> >>> function epilogue (unreachable statement is inserted and the
> > >> >>> return statement is optimised away). Consequently, the
> > >> >>> runtime behaviour is that control is never properly returned
> > >> >>> from this function and thus it starts executing “garbage
> > >> >>> instructions”. As this is undefined behaviour, this is
> > >> >>> perfectly fine and according to the spec, and a compile
> > >> >>> warning for this missing return statement is issued. However,
> > >> >>> in C, the behaviour is that a function epilogue is generated,
> > >> >>> i.e. basically by returning uninitialised local variable.
> > >> >>> Codes that rely on this are not beautiful pieces of code, i.e
> > >> >>> are buggy, but it might just be okay if you for example have
> > >> >>> a function that just initialises stuff (and the return value
> > >> >>> is not checked, directly or indirectly); some one might argue
> > >> >>> that not returning from that function might be a bit harsh.
> > >> >>
> > >> >>
> > >> >> I would not be one of those people.
> > >> >
> > >> >
> > >> > Nor me.
> > >> >>
> > >> >>
> > >> >>>
> > >> >>> So this email is to probe if there would be strong resistance
> > >> >>> to follow the C behaviour? I am not yet sure how, but would
> > >> >>> perhaps a compromise be possible/acceptable to make the
> > >> >>> undefined behaviour explicit and also generate the function
> > >> >>> epilogue?
> > >> >>
> > >> >>
> > >> >> "undefined behavior" is exactly that.
> > >> >>
> > >> >> You have no idea what is going to happen; there are no
> > >> >> restrictions on what the code being executed can do.
> > >> >>
> > >> >> "it just might be ok" means on a particular version of a
> > >> >> particular compiler, on a particular architecture and OS, at a
> > >> >> particular optimization level. Change any of those things, and
> > >> >> you can change the behavior.
> > >> >
> > >> >
> > >> > In fact, the "it works kind of as you expected" is the worst
> > >> > kind of UB in my mind. UB that causes a crash, stops or other
> > >> > "directly obvious that this is wrong" are MUCH easier to debug.
> > >> >
> > >> > So make this particular kind of UB explicit by crashing or
> > >> > stopping would be a good thing. Making it explicit by
> > >> > "returning kind of nicely, but not correct return value" is
> > >> > about the worst possible result.
> > >>
> > >> At -O0 clang emits a trap instruction, making it more explicit as
> > >> you suggest. At higher optimization levels it just falls
> > >> through/off.
> > >>
> > >> >
> > >> > --
> > >> > Mats
> > >> >>
> > >> >>
> > >> >> -- Marshall
> > >> >>
> > >> >>
> > >> >> _______________________________________________
> > >> >> cfe-dev mailing list
> > >> >> cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu>
> > >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> > >> >>
> > >> >
> > >> >
> > >> > _______________________________________________
> > >> > cfe-dev mailing list
> > >> > cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu>
> > >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> > >> >
> > >>
> > >> _______________________________________________
> > >> cfe-dev mailing list
> > >> cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu>
> > >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> >
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu>
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev



-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150814/e1762435/attachment-0001.html>


More information about the cfe-commits mailing list