| Message ID | 20130907082846.GC4841@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
Hi Honza, and thanks for the analysis, now I understand the issue a little more. On 09/07/2013 10:28 AM, Jan Hubicka wrote: > So it is just an accident that the line info is output sanely (if line > 9 is sane, I don't exactly know) I would say that in general it's definitely sane, because that is the instantiation point. And 10 is wrong, too late, it points to the closing bracket. However, I notice that clang doesn't even try to output a message having to do with line 9: if I understand correctly, if that operator cannot be mangled, that happens unconditionally, isn't something that may succeed. Thus I wonder if, instead of outputting garbage line numbers we could at least suppress in such cases the whole "In instantiation of..." part of the diagnostic, it would be better than garbage. Do we have a mechanism for that? Now I also understand that this should be a very uncommon error message, but, I'm wondering, is it possible that other errors, for other issues, are also affected? That is, other diagnostic happening very late and sensitive to the recent rework? Paolo.
> Hi Honza, > > and thanks for the analysis, now I understand the issue a little more. > > On 09/07/2013 10:28 AM, Jan Hubicka wrote: > >So it is just an accident that the line info is output sanely (if > >line 9 is sane, I don't exactly know) > I would say that in general it's definitely sane, because that is > the instantiation point. And 10 is wrong, too late, it points to the > closing bracket. > > However, I notice that clang doesn't even try to output a message > having to do with line 9: if I understand correctly, if that > operator cannot be mangled, that happens unconditionally, isn't > something that may succeed. Thus I wonder if, instead of outputting > garbage line numbers we could at least suppress in such cases the > whole "In instantiation of..." part of the diagnostic, it would be > better than garbage. Do we have a mechanism for that? It seems to be what older GCCs are doing, too. > > Now I also understand that this should be a very uncommon error > message, but, I'm wondering, is it possible that other errors, for > other issues, are also affected? That is, other diagnostic happening > very late and sensitive to the recent rework? Any use of source_location from DECL_ASSEMBLER_NAME hanghook is wrong, since source_location may point anywhere when the lazy mangling is triggered. It surprised me that DECL_ASSEMBLER_NAME triggers error/warning messages. It is up to backend when on at what symbols it will call it, so it is definitely source of inconsistency. So from design point of view I would preffer C++ FE to output these errors somewhere else. But practicaly we can perhaps just modify the message to not expect thelocation? I am just leaving for flight to Pisa, will be back online at the hotel. Honza
Hi >What I can think of is to hide the stale source location when it no >longer have >defined meaning: >Index: cgraphunit.c >=================================================================== >--- cgraphunit.c (revision 202352) >+++ cgraphunit.c (working copy) >@@ -913,6 +913,7 @@ analyze_functions (void) > > bitmap_obstack_initialize (NULL); > cgraph_state = CGRAPH_STATE_CONSTRUCTION; >+ input_location = UNKNOWN_LOCATION; > >/* Ugly, but the fixup can not happen at a time same body alias is >created; > C++ FE is confused about the COMDAT groups being right. */ > >but of course this just leads to: >jh@gcc10:~/trunk/build7/gcc$ ./xgcc -B ./ -O2 >../../gcc/testsuite/g++.dg/template/cond2.C >../../gcc/testsuite/g++.dg/template/cond2.C: In instantiation of 'int >test(c<(X ? : Y)>&) [with int X = 0; int Y = 2]': >:0:0: required from here >../../gcc/testsuite/g++.dg/template/cond2.C:6:28: error: omitted middle >operand to '?:' operand cannot be mangled >template<int X, int Y> int test(c<X ? : Y>&); // { dg-error "omitted" } Actually this kind of change makes a lot of sense to me (cmp clang too): since at that point we do *not* really know the location of the "required from" bit, just plainly admit it. Would it be possible in such cases to have a conditional in the diagnostic machinery and avoid the output of that bit completely when the location is UNKNOWN? I don't think there are existing cases where it's sane to say :0:0 as "required from"!?! Short term at least, it would seem a good enough solution to me + a comment in testcase too. Then indeed as you explained in your last message before leaving (enjoy Pisa! All in all I spent there ~10 years!) we should audit, avoid as much as possible such late diagnostic, etc. What do you think? Paolo
On Sat, Sep 07, 2013 at 11:00:22AM +0200, Paolo Carlini wrote: > and thanks for the analysis, now I understand the issue a little more. > > On 09/07/2013 10:28 AM, Jan Hubicka wrote: > >So it is just an accident that the line info is output sanely (if > >line 9 is sane, I don't exactly know) > I would say that in general it's definitely sane, because that is > the instantiation point. And 10 is wrong, too late, it points to the > closing bracket. > > However, I notice that clang doesn't even try to output a message > having to do with line 9: if I understand correctly, if that > operator cannot be mangled, that happens unconditionally, isn't > something that may succeed. Thus I wonder if, instead of outputting > garbage line numbers we could at least suppress in such cases the > whole "In instantiation of..." part of the diagnostic, it would be > better than garbage. Do we have a mechanism for that? > > Now I also understand that this should be a very uncommon error > message, but, I'm wondering, is it possible that other errors, for > other issues, are also affected? That is, other diagnostic happening > very late and sensitive to the recent rework? As I wrote in the PR, IMHO mangle_decl should location_t save_location = input_location; input_location = DECL_SOURCE_LOCATION (decl); ... input_location = save_location; around the call, and perhaps also all the error calls inside of mangle.c should be actually error_at (EXPR_LOC_OR_HERE (expr), ...), so that if expr has locus, it will print it at that point, otherwise will use locus of the decl. Jakub
On 09/07/2013 06:13 AM, Paolo Carlini wrote:
> Actually this kind of change makes a lot of sense to me (cmp clang too): since at that point we do *not* really know the location of the "required from" bit, just plainly admit it. Would it be possible in such cases to have a conditional in the diagnostic machinery and avoid the output of that bit completely when the location is UNKNOWN? I don't think there are existing cases where it's sane to say :0:0 as "required from"!?!
That does make sense. We should disable "required from" for
UNKNOWN_LOCATION.
Jason
Hi all, Jakub, On 09/07/2013 01:16 PM, Jakub Jelinek wrote: > As I wrote in the PR, IMHO mangle_decl should > location_t save_location = input_location; > input_location = DECL_SOURCE_LOCATION (decl); > ... > input_location = save_location; > around the call, I had a look and I'm afraid this is already happening and isn't enough: in mangle_decl_string, the call of write_mangled_name is wrapped in exactly what you are suggesting. Or you mean something else? Otherwise, I'm afraid we have to resort to what Jason too appears to find acceptable, thus what Honza proposed about UNKNOWN_LOCATION + the tweak to print_instantiation_partial_context_line (essentially just return immediately if loc == UNKNOWN_LOCATION, I think) Paolo.
> Hi all, Jakub, > > On 09/07/2013 01:16 PM, Jakub Jelinek wrote: > >As I wrote in the PR, IMHO mangle_decl should > > location_t save_location = input_location; > > input_location = DECL_SOURCE_LOCATION (decl); > >... > > input_location = save_location; > >around the call, > I had a look and I'm afraid this is already happening and isn't > enough: in mangle_decl_string, the call of write_mangled_name is > wrapped in exactly what you are suggesting. Or you mean something > else? > > Otherwise, I'm afraid we have to resort to what Jason too appears to > find acceptable, thus what Honza proposed about UNKNOWN_LOCATION + > the tweak to print_instantiation_partial_context_line (essentially > just return immediately if loc == UNKNOWN_LOCATION, I think) Yes, I think those two changes makes perfect case. Middle end probably should be more consistent about not setting input_locations beyhond of this point (it does not really make much sense). It is not how things works unforutnately - RTL expansion still lives in statement at a time and uses input location to propagate things down and there are many other random setters of this variable. I will try to audit things incrementally. With the change to cgraphunit I made this week all mangling ought to happen early (this is also not 100% true: in some very special cases where we late access virtual tables through BINFOs, I have independent fix for that). Honza > > Paolo.
> > Hi all, Jakub, > > > > On 09/07/2013 01:16 PM, Jakub Jelinek wrote: > > >As I wrote in the PR, IMHO mangle_decl should > > > location_t save_location = input_location; > > > input_location = DECL_SOURCE_LOCATION (decl); > > >... > > > input_location = save_location; > > >around the call, > > I had a look and I'm afraid this is already happening and isn't > > enough: in mangle_decl_string, the call of write_mangled_name is > > wrapped in exactly what you are suggesting. Or you mean something > > else? > > > > Otherwise, I'm afraid we have to resort to what Jason too appears to > > find acceptable, thus what Honza proposed about UNKNOWN_LOCATION + > > the tweak to print_instantiation_partial_context_line (essentially > > just return immediately if loc == UNKNOWN_LOCATION, I think) > > Yes, I think those two changes makes perfect case. Middle end probably should > be more consistent about not setting input_locations beyhond of this point (it > does not really make much sense). It is not how things works unforutnately - > RTL expansion still lives in statement at a time and uses input location to > propagate things down and there are many other random setters of this variable. > > I will try to audit things incrementally. With the change to cgraphunit I made > this week all mangling ought to happen early (this is also not 100% true: in > some very special cases where we late access virtual tables through BINFOs, I > have independent fix for that). Also I think with David's changes to get rid of global contextes, we probably should have universum (w/o any location), function (with start/end locations as we have in cfun now) and expression (with its own location) contextes now. It would map to our global state, cfun+current_function_decl and input_location+rtl_profile_for_bb in our current state of things. Diagnostic than should know if it binds to specific function or specific statement and can pick the porper location. Honza > > Honza > > > > Paolo.
Index: cgraphunit.c =================================================================== --- cgraphunit.c (revision 202352) +++ cgraphunit.c (working copy) @@ -913,6 +913,7 @@ analyze_functions (void) bitmap_obstack_initialize (NULL); cgraph_state = CGRAPH_STATE_CONSTRUCTION; + input_location = UNKNOWN_LOCATION; /* Ugly, but the fixup can not happen at a time same body alias is created; C++ FE is confused about the COMDAT groups being right. */