[llvm-commits] [llvm-gcc-4.2] r49171 - in /llvm-gcc-4.2/trunk/gcc: llvm-convert.cpp llvm-types.cpp

[Dale Johannesen]

On Apr 4, 2008, at 1:50 AM, Duncan Sands wrote:

> Hi Dale,
>
>> -  if (!tree_could_throw_p(exp))
>> +  if (!tree_could_throw_p(exp) || !flag_exceptions)
>
> this bit is not needed because
>
> bool
> tree_could_throw_p (tree t)
> {
>  if (!flag_exceptions)
>    return false;

OK.

>>   // Compute whether the result needs to be zext or sext'd.
>>   ParameterAttributes RAttributes =  
>> HandleArgumentExtension(ReturnType);
>> +  // Make sure all functions are marked nounwind if we aren't  
>> using exceptions.
>> +  if (!flag_exceptions)
>> +    RAttributes |= ParamAttr::NoUnwind;
>> +
>>   if (RAttributes != ParamAttr::None)
>>     Attrs.push_back(ParamAttrsWithIndex::get(0, RAttributes));
>>
>> @@ -1112,7 +1116,9 @@
>>     RAttributes |= ParamAttr::NoReturn;
>>
>>   // Check for 'nounwind' function attribute.
>> -  if (flags & ECF_NOTHROW)
>> +  // When flag_exceptions is not set (it is set by default in C++/ 
>> ObjC++),
>> +  // don't unwind anything.
>> +  if ((flags & ECF_NOTHROW) || !flag_exceptions)
>
> I think this is wrong, because it means that declarations of  
> external functions
> will always be marked nounwind.  Consider compiling the following  
> without -fexceptions:
>
> extern void g(bool Allowed_To_Throw);
> void f(void) {
>  g(false);
> }
>
> and compiling the following with -fexceptions:
>
> void g(bool Allowed_To_Throw) {
> ...
>  if (Allowed_To_Throw)
>    throw something;
>  else
>    abort();
> ...
> }
>
> then linking the two modules together.  Here g is a function that  
> doesn't throw exceptions
> unless Allowed_To_Throw is true.  With your patch g will be declared  
> nounwind in the first
> module but not in the second.  When you link them g may be marked  
> nounwind in the linked
> module which would be wrong (I don't know if this happens or not  
> currently, but it doesn't
> matter - we shouldn't rely on this kind of linker detail for  
> correctness).

I disagree; this is exactly the sort of detail where the linker  
behavior should be specified.  In fact, if g throws and its caller f  
does not expect it to, that is probably a mistake, and one the linker  
could usefully tell the user about.  (I see that it is not a mistake  
in your example but I think a mistake is much more likely.)

Consider also that llvm-gcc cannot guarantee that all declarations and  
definitions match.  If we do as you suggest, they will mismatch when  
the definition is nounwind.

As for the linker behavior, I would expect the definition to win.   
Right now running your example through llvm-ld does appear to mark g  
as nounwind, even though it throws.  I believe the appropriate thing  
to do is change the linker to take the nounwind bit from the  
definition, and perhaps emit a warning.

> What I think should happen is:
>
> (1) *calls* to g from the first module should be marked nounwind.   
> In fact all calls in
> the first module should be marked nounwind.  This is already the  
> case thanks to tree_could_throw_p.
> (2) external declarations should not be marked nounwind.
> (3) functions like f with a body compiled without -fexceptions  
> should be marked nounwind.
>
> So I think you should undo these changes, and instead add some code  
> to StartFunctionBody
> to add the nounwind attribute if !flag_exceptions.
>
> Best wishes,
>
> Duncan.
>
> PS: I was wrong to think that llvm-gcc was already marking functions  
> nounwind when compiled
> with !flag_exceptions - it was prune_eh that was adding the  
> marking.  Nice catch!

It was sometimes, but not in all cases.