Re: [patch] Fix PR 23439/23440

[Ian Lance Taylor <ian at airs dot com>]

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.

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);
}

Then I think we should add at the start of annotate_with_file_line
  gcc_assert (file);

Anybody see any problems with that approach?

Ian