Add extra location information - PR43486

> Sorry for picking on simple stuff, but the switch name seems

No problem, and thanks for your feedback.

> meaningless, and there isn't any documentation.

Ah. I'm open for suggestion on a better name, or I can come up with a
new one.

I'll indeed add documentation as soon as there's some kind of agreement on
the approach.

> Conceptually it looks like you are trying to make up for the absence
> of a proper AST by building an on-the-side hash table to track
> expression locations.

Right, that's the idea.

> The hash table key is the tree structure
> itself.

Yes.

> The thing is, any call into fold-const may give you an
> entirely new tree,

Exactly.

> and at that point you have lost your extra location
> information.

Actually no, see the c-family/c-common.c patch, copied here, which
ensures that folding does preserve such information:

	* c-common.c (c_fully_fold_internal): Copy extra locations on new node.

> And the C/C++ frontends call into fold-const regularly,
> which is why we don't have a proper AST in the first place.  So it
> seems to me that this is going to be kind of frustrating, in that we
> will often have the extra location information but sometimes we won't.

That's not the case as per the c-common.c patch, the locations are preserved
across fold, otherwise as you said, the whole approach would be pretty useless.

I should perhaps have mentioned that this patch (and the -fdump-xref
implementation on top of it) has been in production in our (AdaCore) tree for
more than 2 years now, with pretty good results, and certainly most expressions
trees do have extra sloc info available in our experience.

>  And whether we have it or not will change as the frontends change.

See above.

Does this address your concern?

Arno

On Tue, Sep 18, 2012 at 10:58 AM, Arnaud Charlet <charlet@adacore.com> wrote:
>
>> and at that point you have lost your extra location
>> information.
>
> Actually no, see the c-family/c-common.c patch, copied here, which
> ensures that folding does preserve such information:

Thanks.  I think I would like some clarity on when the extra location
information is available.  For better or for worse the C frontend does
sometimes call directly into fold-const, without going through
c_fully_fold.  E.g., I see calls to fold_convert and fold_build2_loc.
What happens then?

Ian

> >> and at that point you have lost your extra location
> >> information.
> >
> > Actually no, see the c-family/c-common.c patch, copied here, which
> > ensures that folding does preserve such information:
> 
> Thanks.  I think I would like some clarity on when the extra location
> information is available.

Typically the extra information would be used right before gimplification,
just after the front-end has done its job (e.g. via the PLUGIN_PRE_GENERICIZE
hooks if implemented via a plug-in). It could also be used during the
front-end itself, to e.g. generate more accurate error messages or warnings.

> For better or for worse the C frontend does
> sometimes call directly into fold-const, without going through
> c_fully_fold.

Ah yes... I can't resist but note the following comment in fold_convert_loc:
--
Used by the middle-end for
simple conversions in preference to calling the front-end's convert.
--

Not only used by the middle-end apparently... So I guess we should first
clarify whether this is an API/layering violation.

> E.g., I see calls to fold_convert and fold_build2_loc.
> What happens then?

I've looked at most of these calls in the C and C++ front-end, and I suspect
most of these calls (e.g. most calls to fold_build2_loc) correspond
to internally generated expressions, not directly relevant to the source code.

In other words, my patch aims at providing more detailed slocs
(source locations) for the source representation, so that e.g. static
analysis tools, plug-ins, diagnostic tools (potentially error messages)
have more precise source location info.

In a few cases of calls to fold_convert_loc/fold_build2_loc/etc... I guess we
might indeed loose some sloc info, to be confirmed. In which case, depending
whether e.g. calling fold_convert_loc() is indeed expected or not, we could
refine the approach to not loose this information.

Also note that we are talking about very few cases, and the idea behind the
patch is to provide extra info as much as possible, but there's always an
available fallback, which is the main source location. In other words, this
approach can be incremental, and does not need to be "complete" to become
useful.

Arno