Patchwork crash fix for unhanded operation

login
register
mail settings
Submitter Mike Stump
Date Sept. 9, 2013, 9:32 p.m.
Message ID <44C327AE-2328-4322-B792-61F22DFB1588@comcast.net>
Download mbox | patch
Permalink /patch/273711/
State New
Headers show

Comments

Mike Stump - Sept. 9, 2013, 9:32 p.m.
Presently gcc just dies with a crash for an unhanded operation, the below handles it better.

I'm torn between sorry and error, error might be better.  Thoughts?

Ok?
Joseph S. Myers - Sept. 9, 2013, 10:48 p.m.
On Mon, 9 Sep 2013, Mike Stump wrote:

> Presently gcc just dies with a crash for an unhanded operation, the 
> below handles it better.
> 
> I'm torn between sorry and error, error might be better.  Thoughts?

error means there is something wrong with the user's source code, and 
should generally be associated with the location of an erroneous source 
code construct.  I don't see how it can be appropriate here; my impression 
is that this code should never fail for any compiler input, valid or 
invalid, and so an ICE seems better than sorry (which is for some 
well-defined source code feature lacking GCC support, or exceeding 
implementation limits in GCC).
Mike Stump - Sept. 9, 2013, 11:49 p.m.
On Sep 9, 2013, at 3:48 PM, "Joseph S. Myers" <joseph@codesourcery.com> wrote:
> On Mon, 9 Sep 2013, Mike Stump wrote:
> 
>> Presently gcc just dies with a crash for an unhanded operation, the 
>> below handles it better.
>> 
>> I'm torn between sorry and error, error might be better.  Thoughts?
> 
> error means there is something wrong with the user's source code,

Right.  If one has mode X, and there are no instructions for mode X, the thing that is wrong with their source code is wanting to perform an operation in mode X.  The language standard doesn't mandate that the operation for mode X works, so, we are free to just give an error.

> should generally be associated with the location of an erroneous source 
> code construct.

Indeed, the ^ points to exactly what is wrong in their source, which is (relatively new and) nice.

> I don't see how it can be appropriate here; my impression 
> is that this code should never fail for any compiler input,

Your impression is wrong.  It does indeed fail.
Mike Stump - Sept. 10, 2013, 4:49 p.m.
On Sep 9, 2013, at 4:49 PM, Mike Stump <mikestump@comcast.net> wrote:
>> I don't see how it can be appropriate here; my impression 
>> is that this code should never fail for any compiler input,

Oh, I might have missed what you meant by this…  If so, sorry…  The nicest of options would be to generate a libcall for the missing operation…  if we do that, then there would be no failure to compile and we can expect the runtime to put the operation in.
Joseph S. Myers - Sept. 10, 2013, 10:43 p.m.
On Mon, 9 Sep 2013, Mike Stump wrote:

> On Sep 9, 2013, at 3:48 PM, "Joseph S. Myers" <joseph@codesourcery.com> wrote:
> > On Mon, 9 Sep 2013, Mike Stump wrote:
> > 
> >> Presently gcc just dies with a crash for an unhanded operation, the 
> >> below handles it better.
> >> 
> >> I'm torn between sorry and error, error might be better.  Thoughts?
> > 
> > error means there is something wrong with the user's source code,
> 
> Right.  If one has mode X, and there are no instructions for mode X, the 
> thing that is wrong with their source code is wanting to perform an 
> operation in mode X.  The language standard doesn't mandate that the 
> operation for mode X works, so, we are free to just give an error.

Well, your patch was missing the testcase, or explanation for why a 
testcase can't be added to the testsuite, so I don't know what sort of 
source code you have in mind here.  But "mode" is only a source-code 
concept to the extent that the user uses "mode" attributes.

If the user does a division of "int" values, say, that's fully defined by 
ISO C, and so if there's back-end support for it missing, that's a 
back-end bug and an ICE is appropriate; it doesn't reflect anything wrong 
with the user's source code.  If however the user used a "mode" attribute 
for a mode for which the division operation is unsupported, the front end 
should give an error for that operation, just as the front end gives an 
error for dividing two struct values (unless of course that's supported by 
the language, in which case it gets lowered to appropriate GIMPLE); it 
doesn't generate GIMPLE with a struct division because that's invalid 
GIMPLE, and it shouldn't generate GIMPLE with a division on types for 
which the operation isn't supported.  Note that for 32-bit x86, simply 
doing

int x __attribute__((mode(TI)));

gives "error: unable to emulate 'TI'"; you don't even need to try any 
operations on that mode.  For target-specific types with more fine-grained 
restrictions on permitted operations, there are several target hooks such 
as invalid_unary_op, invalid_binary_op and invalid_parameter_type.  That's 
how the errors should be given, so that the invalid GIMPLE is never 
generated.

> > should generally be associated with the location of an erroneous source 
> > code construct.
> 
> Indeed, the ^ points to exactly what is wrong in their source, which is 
> (relatively new and) nice.

My observation has been that errors given by RTL expanders are at bad 
locations because input_location at that point tends to be set to the end 
of the function (at best).  In particular, I've seen that for errors from 
target-specific built-in function expanders.  But maybe this has improved 
since I saw such bad locations.
Mike Stump - Sept. 11, 2013, 6:54 p.m.
On Sep 10, 2013, at 3:43 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> For target-specific types with more fine-grained 
> restrictions on permitted operations, there are several target hooks such 
> as invalid_unary_op, invalid_binary_op and invalid_parameter_type.  That's 
> how the errors should be given, so that the invalid GIMPLE is never 
> generated.

Ah…  yes, that does the trick, however, the disconnect between rtl and gimple is annoying.  gimply (or rtl) is free to decompose operations, for example, xor can be decomposed into n independent xors of the parts of a larger piece of data, and if it does that, then the port should not give an error, and if gimple is does not do this, then the port should, but, the port can't know to do this or not, and to retain the flexibility to allow gimple lowering to improve over time.

As a concrete example, xor, or, gt, ge, lt, le lower on plain integer modes; but eq and ne don't.  Odd that.  I'd claim it is a mere implementation detail of lowering and requiring port work for an internal implementation detail is odd.

But, with the interfaces you mentioned I can solve the problem…  I'll plan on doing it that way, not ideal, but reasonable.

Thanks for the help.

Patch

diff --git a/gcc/expmed.c b/gcc/expmed.c
index a83d549..127085f 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -4954,6 +4954,12 @@  expand_divmod (int rem_flag, enum tree_code code, enum machine_mode mode,
 	}
     }
 
+  if (rem_flag ? (remainder == 0) : (quotient == 0))
+    {
+      sorry ("operation not supported");
+      return CONST0_RTX (mode);
+    }
+
   return gen_lowpart (mode, rem_flag ? remainder : quotient);
 }