Re: [patch] Fix PR 23439/23440

[Volker Reichelt <reichelt at igpm dot rwth-aachen dot de>]

On 15 Sep, Ian Lance Taylor wrote:
> Daniel Berlin <dberlin@dberlin.org> writes:
> 
>> > One could try to fix this in the callers of annotate_with_file_line,
>> > but to be sure that all cases are caught, I'd prefer to fix
>> > annotate_with_file_line.
>> 
>> ISTM a better solution would be to fix the callers, and add an assert
>> that the argument isn't null to make sure "all cases are caught".
>> 
>> You shouldn't have to continually workaround people calling functions
>> with bad input.  Adding these types of "if someone called us with bad
>> arguments, return early" tests everywhere is death by 1000 pinpricks in
>> terms of making gcc slower.
> 
> The code in question is the result of
>     SET_EXPR_LOCATION (node, UNKNOWN_LOCATION);
> 
> That works fine when configured with --enable-mapped-location, because
> then UNKNOWN_LOCATION is NULL and that expands into
>     node->exp.locus = NULL;
> 
> Without --enable-mapped-location (the current default, perhaps to be
> eliminated later) UNKNOWN_LOCATION is the global variable
> unknown_location defined in toplev.c, and the SET_EXPR_LOCATION turns
> into
>     annotate_with_locus (node, unknown_location);
> which turns into
>     annotate_with_file_line (node, NULL, 0);
> 
> The point is that annotate_with_file_line is not the problem here.

No, annotate_with_file_line *is* the problem.
It just cannot handle UNKNOWN_LOCATION.
 
> Do we want to permit SET_EXPR_LOCATION (node, UNKNOWN_LOCATION)?  It
> seems to me that we do, as it is simpler to permit it than to always
> detect it.  Therefore, I think a correct patch here is to change
> 
> void
> annotate_with_locus (tree node, location_t locus)
> {
>   annotate_with_file_line (node, locus.file, locus.line);
> }
> 
> to
> 
> void
> annotate_with_locus (tree node, location_t locus)
> {
>   if (locus.file == NULL && locus.line == 0)
>     SET_EXPR_LOCUS (node, NULL);
>   else
>     annotate_with_file_line (node, locus.file, locus.line);
> }

Given that annotate_with_locus is (almost) the only caller of
annotate_with_file_line this is roughly the same as adding a
check on the top of a_w_f_l:

  if (!file)
    SET_EXPR_LOCUS (node, NULL);

In the case that the node has no annotation yet, this is the
same as in my version

  if (!file)
    return;

If it alredy has an annotation, the latter version keeps the original
annotation and the previous version deletes it. I'm fine with both,
but then I'd rather have the check in a_w_f_l instead of hiding
it in annotate_with_locus.

> Then I think we should add at the start of annotate_with_file_line
>   gcc_assert (file);
> 
> Anybody see any problems with that approach?

Apart from "SET_EXPR_LOCUS (node, NULL);" vs. "return;" I fail
to see a real difference between this and my version.

> Ian

The real question IMHO is still the one from
http://gcc.gnu.org/ml/gcc-patches/2005-09/msg00950.html :
Is UNKNOWN_LOCATION broken or is a_w_f_l broken?

Regards,
Volker