* [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076]
@ 2020-07-28 19:50 Patrick Palka
2020-07-28 20:52 ` David Malcolm
2020-08-10 13:46 ` Patrick Palka
0 siblings, 2 replies; 8+ messages in thread
From: Patrick Palka @ 2020-07-28 19:50 UTC (permalink / raw)
To: gcc-patches
Currently the -Wmisleading-indentation warning doesn't do any analysis
when the guarded statement or the statement after it is produced by a
macro, as the mentioned PR illustrates. This means that we warn for:
if (flag)
foo ();
bar ();
and yet we don't warn for:
#define BAR bar
if (flag)
foo ();
BAR ();
which seems like a surprising limitation.
This patch extends the -Wmisleading-indentation implementation to
support analyzing such statements and their tokens. This is done in the
"natural" way by resolving the location of each of the three tokens to
the token's macro expansion point. (Additionally, if the tokens all
resolve to the same macro expansion point then we instead use their
locations within the macro definition.) When these resolved locations
are all different, then we can proceed with applying the warning
heuristics to them as if no macros were involved.
Bootstrapped and regtested on x86_64-pc-linux-gnu,
powerpc64le-unknown-linux-gnu, aarch64-unknown-linux-gnu and
s390x-ibm-linux-gnu.
I also built sqlite3, glibc, linux, and a number of other projects with
this patch, to help verify that it doesn't introduce bogus warnings.
All new warnings that I've seen look justified from what I can tell,
e.g. we now warn about the following line, which is indented as if it's
guarded by the if statement two lines above it:
https://github.com/torvalds/linux/blob/fe557319aa06c23cffc9346000f119547e0f289a/drivers/misc/kgdbts.c#L105
Does this look OK to commit?
gcc/c-family/ChangeLog:
PR c/80076
* c-indentation.c (should_warn_for_misleading_indentation): Move
declarations of local variables closer to their first use.
Handle virtual token locations by resolving them to their
respective macro expansion points. If all three tokens are
produced from the same macro expansion, then instead use their
loci within the macro definition.
gcc/objc/ChangeLog:
PR c/80076
* objc-gnu-runtime-abi-01.c
(gnu_runtime_abi_01_get_class_super_ref): Reduce indentation of
misleadingly indented return statements.
* objc-next-runtime-abi-01.c
(next_runtime_abi_01_get_class_super_ref): Likewise.
gcc/ChangeLog:
PR c/80076
* gensupport.c (alter_attrs_for_subst_insn) <case SET_ATTR>:
Reduce indentation of misleadingly indented code fragment.
* lra-constraints.c (multi_block_pseudo_p): Likewise.
* sel-sched-ir.c (merge_fences): Likewise.
libcpp/ChangeLog:
PR c/80076
* include/line-map.h (first_map_in_common): Declare.
* line-map.c (first_map_in_common): Remove static.
gcc/testsuite/ChangeLog:
PR c/80076
* c-c++-common/Wmisleading-indentation-pr80076.c: New test.
---
gcc/c-family/c-indentation.c | 61 ++++++++--
gcc/gensupport.c | 2 +-
gcc/lra-constraints.c | 12 +-
gcc/objc/objc-gnu-runtime-abi-01.c | 4 +-
gcc/objc/objc-next-runtime-abi-01.c | 4 +-
gcc/sel-sched-ir.c | 112 +++++++++---------
.../c-c++-common/Wmisleading-indentation-5.c | 56 +++++++++
libcpp/include/line-map.h | 6 +
libcpp/line-map.c | 2 +-
9 files changed, 178 insertions(+), 81 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index d814f6f29e6..8b88a8adc7c 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -213,19 +213,6 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
const token_indent_info &body_tinfo,
const token_indent_info &next_tinfo)
{
- location_t guard_loc = guard_tinfo.location;
- location_t body_loc = body_tinfo.location;
- location_t next_stmt_loc = next_tinfo.location;
-
- enum cpp_ttype body_type = body_tinfo.type;
- enum cpp_ttype next_tok_type = next_tinfo.type;
-
- /* Don't attempt to compare the indentation of BODY_LOC and NEXT_STMT_LOC
- if either are within macros. */
- if (linemap_location_from_macro_expansion_p (line_table, body_loc)
- || linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
- return false;
-
/* Don't attempt to compare indentation if #line or # 44 "file"-style
directives are present, suggesting generated code.
@@ -266,6 +253,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
} <- NEXT
baz ();
*/
+ enum cpp_ttype next_tok_type = next_tinfo.type;
if (next_tok_type == CPP_CLOSE_BRACE
|| next_tinfo.keyword == RID_ELSE)
return false;
@@ -287,6 +275,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
bar (); <- BODY
baz (); <- NEXT
*/
+ enum cpp_ttype body_type = body_tinfo.type;
if (body_type == CPP_OPEN_BRACE)
return false;
@@ -294,6 +283,52 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
if (next_tok_type == CPP_SEMICOLON)
return false;
+ location_t guard_loc = guard_tinfo.location;
+ location_t body_loc = body_tinfo.location;
+ location_t next_stmt_loc = next_tinfo.location;
+
+ /* Resolve each token location to the respective macro expansion
+ point that produced the token. */
+ if (linemap_location_from_macro_expansion_p (line_table, guard_loc))
+ guard_loc = linemap_resolve_location (line_table, guard_loc,
+ LRK_MACRO_EXPANSION_POINT, NULL);
+ if (linemap_location_from_macro_expansion_p (line_table, body_loc))
+ body_loc = linemap_resolve_location (line_table, body_loc,
+ LRK_MACRO_EXPANSION_POINT, NULL);
+ if (linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
+ next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
+ LRK_MACRO_EXPANSION_POINT, NULL);
+
+ /* When all three tokens are produced from a single macro expansion, we
+ instead consider their loci inside that macro's definition. */
+ if (guard_loc == body_loc && body_loc == next_stmt_loc)
+ {
+ const line_map *guard_body_common_map
+ = first_map_in_common (line_table,
+ guard_tinfo.location, body_tinfo.location,
+ &guard_loc, &body_loc);
+ const line_map *body_next_common_map
+ = first_map_in_common (line_table,
+ body_tinfo.location, next_tinfo.location,
+ &body_loc, &next_stmt_loc);
+
+ /* Punt on complicated nesting of macros. */
+ if (guard_body_common_map != body_next_common_map)
+ return false;
+
+ guard_loc = linemap_resolve_location (line_table, guard_loc,
+ LRK_MACRO_DEFINITION_LOCATION, NULL);
+ body_loc = linemap_resolve_location (line_table, body_loc,
+ LRK_MACRO_DEFINITION_LOCATION, NULL);
+ next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
+ LRK_MACRO_DEFINITION_LOCATION,
+ NULL);
+ }
+
+ /* Give up if the loci are not all distinct. */
+ if (guard_loc == body_loc || body_loc == next_stmt_loc)
+ return false;
+
expanded_location body_exploc = expand_location (body_loc);
expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
expanded_location guard_exploc = expand_location (guard_loc);
diff --git a/gcc/gensupport.c b/gcc/gensupport.c
index f2ad54f0c55..61691aadff1 100644
--- a/gcc/gensupport.c
+++ b/gcc/gensupport.c
@@ -1501,7 +1501,7 @@ alter_attrs_for_subst_insn (class queue_elem * elem, int n_dup)
case SET_ATTR:
if (strchr (XSTR (sub, 1), ',') != NULL)
XSTR (sub, 1) = duplicate_alternatives (XSTR (sub, 1), n_dup);
- break;
+ break;
case SET_ATTR_ALTERNATIVE:
case SET:
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 421c453997b..90ca31632f1 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -4707,12 +4707,12 @@ multi_block_pseudo_p (int regno)
if (regno < FIRST_PSEUDO_REGISTER)
return false;
- EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
- if (bb == NULL)
- bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
- else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
- return true;
- return false;
+ EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
+ if (bb == NULL)
+ bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
+ else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
+ return true;
+ return false;
}
/* Return true if LIST contains a deleted insn. */
diff --git a/gcc/objc/objc-gnu-runtime-abi-01.c b/gcc/objc/objc-gnu-runtime-abi-01.c
index d5862435c29..c9959a7e1e8 100644
--- a/gcc/objc/objc-gnu-runtime-abi-01.c
+++ b/gcc/objc/objc-gnu-runtime-abi-01.c
@@ -821,7 +821,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
ucls_super_ref =
objc_build_component_ref (imp->class_decl,
get_identifier ("super_class"));
- return ucls_super_ref;
+ return ucls_super_ref;
}
else
{
@@ -829,7 +829,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
uucls_super_ref =
objc_build_component_ref (imp->meta_decl,
get_identifier ("super_class"));
- return uucls_super_ref;
+ return uucls_super_ref;
}
}
diff --git a/gcc/objc/objc-next-runtime-abi-01.c b/gcc/objc/objc-next-runtime-abi-01.c
index 5c34fcb05cb..233d89e75b5 100644
--- a/gcc/objc/objc-next-runtime-abi-01.c
+++ b/gcc/objc/objc-next-runtime-abi-01.c
@@ -938,7 +938,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
ucls_super_ref =
objc_build_component_ref (imp->class_decl,
get_identifier ("super_class"));
- return ucls_super_ref;
+ return ucls_super_ref;
}
else
{
@@ -946,7 +946,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
uucls_super_ref =
objc_build_component_ref (imp->meta_decl,
get_identifier ("super_class"));
- return uucls_super_ref;
+ return uucls_super_ref;
}
}
diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 4f1e4afdc4c..1dd981ff8d5 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -722,63 +722,63 @@ merge_fences (fence_t f, insn_t insn,
!= BLOCK_FOR_INSN (last_scheduled_insn));
}
- /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
- FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
- SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
- {
- if (succ == insn)
- {
- /* No same successor allowed from several edges. */
- gcc_assert (!edge_old);
- edge_old = si.e1;
- }
- }
- /* Find edge of second predecessor (last_scheduled_insn->insn). */
- FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
- SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
- {
- if (succ == insn)
- {
- /* No same successor allowed from several edges. */
- gcc_assert (!edge_new);
- edge_new = si.e1;
- }
- }
+ /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
+ FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
+ SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
+ {
+ if (succ == insn)
+ {
+ /* No same successor allowed from several edges. */
+ gcc_assert (!edge_old);
+ edge_old = si.e1;
+ }
+ }
+ /* Find edge of second predecessor (last_scheduled_insn->insn). */
+ FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
+ SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
+ {
+ if (succ == insn)
+ {
+ /* No same successor allowed from several edges. */
+ gcc_assert (!edge_new);
+ edge_new = si.e1;
+ }
+ }
- /* Check if we can choose most probable predecessor. */
- if (edge_old == NULL || edge_new == NULL)
- {
- reset_deps_context (FENCE_DC (f));
- delete_deps_context (dc);
- vec_free (executing_insns);
- free (ready_ticks);
-
- FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
- if (FENCE_EXECUTING_INSNS (f))
- FENCE_EXECUTING_INSNS (f)->block_remove (0,
- FENCE_EXECUTING_INSNS (f)->length ());
- if (FENCE_READY_TICKS (f))
- memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
- }
- else
- if (edge_new->probability > edge_old->probability)
- {
- delete_deps_context (FENCE_DC (f));
- FENCE_DC (f) = dc;
- vec_free (FENCE_EXECUTING_INSNS (f));
- FENCE_EXECUTING_INSNS (f) = executing_insns;
- free (FENCE_READY_TICKS (f));
- FENCE_READY_TICKS (f) = ready_ticks;
- FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
- FENCE_CYCLE (f) = cycle;
- }
- else
- {
- /* Leave DC and CYCLE untouched. */
- delete_deps_context (dc);
- vec_free (executing_insns);
- free (ready_ticks);
- }
+ /* Check if we can choose most probable predecessor. */
+ if (edge_old == NULL || edge_new == NULL)
+ {
+ reset_deps_context (FENCE_DC (f));
+ delete_deps_context (dc);
+ vec_free (executing_insns);
+ free (ready_ticks);
+
+ FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
+ if (FENCE_EXECUTING_INSNS (f))
+ FENCE_EXECUTING_INSNS (f)->block_remove (0,
+ FENCE_EXECUTING_INSNS (f)->length ());
+ if (FENCE_READY_TICKS (f))
+ memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
+ }
+ else
+ if (edge_new->probability > edge_old->probability)
+ {
+ delete_deps_context (FENCE_DC (f));
+ FENCE_DC (f) = dc;
+ vec_free (FENCE_EXECUTING_INSNS (f));
+ FENCE_EXECUTING_INSNS (f) = executing_insns;
+ free (FENCE_READY_TICKS (f));
+ FENCE_READY_TICKS (f) = ready_ticks;
+ FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
+ FENCE_CYCLE (f) = cycle;
+ }
+ else
+ {
+ /* Leave DC and CYCLE untouched. */
+ delete_deps_context (dc);
+ vec_free (executing_insns);
+ free (ready_ticks);
+ }
}
/* Fill remaining invariant fields. */
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
new file mode 100644
index 00000000000..12b53569ba7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
@@ -0,0 +1,56 @@
+/* PR c/80076 */
+/* { dg-options "-Wmisleading-indentation" } */
+
+void foo(void);
+
+void test01(int flag) {
+#define bar() foo() /* { dg-message "this statement" } */
+ if (flag) /* { dg-warning "does not guard" } */
+ foo();
+ bar(); /* { dg-message "in expansion of macro" } */
+#undef bar
+}
+
+void test02(int flag) {
+#define bar() foo()
+ if (flag) /* { dg-warning "does not guard" } */
+ bar();
+ foo(); /* { dg-message "this statement" } */
+#undef bar
+}
+
+void test03(int flag) {
+#define bar() foo() /* { dg-message "this statement" } */
+ if (flag) /* { dg-warning "does not guard" } */
+ bar();
+ bar(); /* { dg-message "in expansion of macro" } */
+#undef bar
+}
+
+void test04(int flag, int num) {
+#define bar() \
+ { \
+ if (flag) \
+ num = 0; \
+ num = 1; \
+ }
+ bar();
+/* { dg-warning "does not guard" "" { target *-*-* } .-5 } */
+/* { dg-message "this statement" "" { target *-*-* } .-4 } */
+#undef bar
+}
+
+void test05(int flag, int num) {
+#define baz() (num = 1)
+#define bar() \
+ { \
+ if (flag) \
+ num = 0; \
+ baz(); \
+ }
+#define wrapper bar
+ wrapper();
+/* { dg-warning "does not guard" "" { target *-*-* } .-6 } */
+/* { dg-message "this statement" "" { target *-*-* } .-10 } */
+#undef bar
+}
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 217f916fc35..44008be5c08 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1225,6 +1225,12 @@ LINEMAP_SYSP (const line_map_ordinary *ord_map)
return ord_map->sysp;
}
+const struct line_map *first_map_in_common (line_maps *set,
+ location_t loc0,
+ location_t loc1,
+ location_t *res_loc0,
+ location_t *res_loc1);
+
/* Return a positive value if PRE denotes the location of a token that
comes before the token of POST, 0 if PRE denotes the location of
the same token as the token for POST, and a negative value
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index a8d52861dee..5a74174579f 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -1289,7 +1289,7 @@ first_map_in_common_1 (line_maps *set,
virtual location of the token inside the resulting macro, upon
return of a non-NULL result. */
-static const struct line_map*
+const struct line_map*
first_map_in_common (line_maps *set,
location_t loc0,
location_t loc1,
--
2.28.0.rc1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076]
2020-07-28 19:50 [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076] Patrick Palka
@ 2020-07-28 20:52 ` David Malcolm
2020-07-29 0:22 ` Patrick Palka
2020-08-10 13:46 ` Patrick Palka
1 sibling, 1 reply; 8+ messages in thread
From: David Malcolm @ 2020-07-28 20:52 UTC (permalink / raw)
To: Patrick Palka, gcc-patches
On Tue, 2020-07-28 at 15:50 -0400, Patrick Palka wrote:
> Currently the -Wmisleading-indentation warning doesn't do any
> analysis
> when the guarded statement or the statement after it is produced by a
> macro, as the mentioned PR illustrates. This means that we warn for:
>
> if (flag)
> foo ();
> bar ();
>
> and yet we don't warn for:
>
> #define BAR bar
> if (flag)
> foo ();
> BAR ();
>
> which seems like a surprising limitation.
IIRC we were trying to keep things simple in the initial
implementation.
> This patch extends the -Wmisleading-indentation implementation to
> support analyzing such statements and their tokens. This is done in
> the
> "natural" way by resolving the location of each of the three tokens
> to
> the token's macro expansion point. (Additionally, if the tokens all
> resolve to the same macro expansion point then we instead use their
> locations within the macro definition.) When these resolved
> locations
> are all different, then we can proceed with applying the warning
> heuristics to them as if no macros were involved.
Thanks for working on this. I've only looked at the patch briefly, but
so far it looks reasonable.
Can you post some examples of what the output looks like for some of
these cases? The diagnostics code has some logic for printing how
macros are unwound, so I'm wondering what we actually print for these
cases, and if it looks intelligble to an end-user.
Dave
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076]
2020-07-28 20:52 ` David Malcolm
@ 2020-07-29 0:22 ` Patrick Palka
2020-09-10 17:01 ` David Malcolm
0 siblings, 1 reply; 8+ messages in thread
From: Patrick Palka @ 2020-07-29 0:22 UTC (permalink / raw)
To: David Malcolm; +Cc: Patrick Palka, gcc-patches
On Tue, 28 Jul 2020, David Malcolm wrote:
> On Tue, 2020-07-28 at 15:50 -0400, Patrick Palka wrote:
> > Currently the -Wmisleading-indentation warning doesn't do any
> > analysis
> > when the guarded statement or the statement after it is produced by a
> > macro, as the mentioned PR illustrates. This means that we warn for:
> >
> > if (flag)
> > foo ();
> > bar ();
> >
> > and yet we don't warn for:
> >
> > #define BAR bar
> > if (flag)
> > foo ();
> > BAR ();
> >
> > which seems like a surprising limitation.
>
> IIRC we were trying to keep things simple in the initial
> implementation.
>
> > This patch extends the -Wmisleading-indentation implementation to
> > support analyzing such statements and their tokens. This is done in
> > the
> > "natural" way by resolving the location of each of the three tokens
> > to
> > the token's macro expansion point. (Additionally, if the tokens all
> > resolve to the same macro expansion point then we instead use their
> > locations within the macro definition.) When these resolved
> > locations
> > are all different, then we can proceed with applying the warning
> > heuristics to them as if no macros were involved.
>
> Thanks for working on this. I've only looked at the patch briefly, but
> so far it looks reasonable.
>
> Can you post some examples of what the output looks like for some of
> these cases? The diagnostics code has some logic for printing how
> macros are unwound, so I'm wondering what we actually print for these
> cases, and if it looks intelligble to an end-user.
For the code fragment mentioned above:
int
main()
{
#define BAR
if (flag)
foo ();
BAR ();
}
we emit:
test.cc: In function ‘int main()’:
test.cc:7:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
9 | if (flag)
| ^~
test.cc:8:15: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
8 | #define BAR bar
| ^~~
test.cc:10:5: note: in expansion of macro ‘BAR’
11 | BAR ();
| ^~~
And when all tokens are generated from the same macro, e.g. for:
int
main()
{
#define BAR bar
#define DO_STUFF \
if (flag) \
foo (); \
BAR ();
DO_STUFF;
}
we emit:
test.cc: In function ‘int main()’:
test.cc:11:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
11 | if (flag) \
| ^~
test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
14 | DO_STUFF;
| ^~~~~~~~
test.cc:9:15: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
9 | #define BAR bar
| ^~~
test.cc:13:7: note: in expansion of macro ‘BAR’
13 | BAR ();
| ^~~
test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
14 | DO_STUFF;
| ^~~~~~~~
The automatic macro unwinding logic saves the day here and yields mostly
legible output "for free". What do you think?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076]
2020-07-28 19:50 [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076] Patrick Palka
2020-07-28 20:52 ` David Malcolm
@ 2020-08-10 13:46 ` Patrick Palka
2020-09-10 13:40 ` Patrick Palka
1 sibling, 1 reply; 8+ messages in thread
From: Patrick Palka @ 2020-08-10 13:46 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches, dmalcolm
On Tue, 28 Jul 2020, Patrick Palka wrote:
> Currently the -Wmisleading-indentation warning doesn't do any analysis
> when the guarded statement or the statement after it is produced by a
> macro, as the mentioned PR illustrates. This means that we warn for:
>
> if (flag)
> foo ();
> bar ();
>
> and yet we don't warn for:
>
> #define BAR bar
> if (flag)
> foo ();
> BAR ();
>
> which seems like a surprising limitation.
>
> This patch extends the -Wmisleading-indentation implementation to
> support analyzing such statements and their tokens. This is done in the
> "natural" way by resolving the location of each of the three tokens to
> the token's macro expansion point. (Additionally, if the tokens all
> resolve to the same macro expansion point then we instead use their
> locations within the macro definition.) When these resolved locations
> are all different, then we can proceed with applying the warning
> heuristics to them as if no macros were involved.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu,
> powerpc64le-unknown-linux-gnu, aarch64-unknown-linux-gnu and
> s390x-ibm-linux-gnu.
>
> I also built sqlite3, glibc, linux, and a number of other projects with
> this patch, to help verify that it doesn't introduce bogus warnings.
> All new warnings that I've seen look justified from what I can tell,
> e.g. we now warn about the following line, which is indented as if it's
> guarded by the if statement two lines above it:
> https://github.com/torvalds/linux/blob/fe557319aa06c23cffc9346000f119547e0f289a/drivers/misc/kgdbts.c#L105
>
> Does this look OK to commit?
>
> gcc/c-family/ChangeLog:
>
> PR c/80076
> * c-indentation.c (should_warn_for_misleading_indentation): Move
> declarations of local variables closer to their first use.
> Handle virtual token locations by resolving them to their
> respective macro expansion points. If all three tokens are
> produced from the same macro expansion, then instead use their
> loci within the macro definition.
>
> gcc/objc/ChangeLog:
>
> PR c/80076
> * objc-gnu-runtime-abi-01.c
> (gnu_runtime_abi_01_get_class_super_ref): Reduce indentation of
> misleadingly indented return statements.
> * objc-next-runtime-abi-01.c
> (next_runtime_abi_01_get_class_super_ref): Likewise.
>
> gcc/ChangeLog:
>
> PR c/80076
> * gensupport.c (alter_attrs_for_subst_insn) <case SET_ATTR>:
> Reduce indentation of misleadingly indented code fragment.
> * lra-constraints.c (multi_block_pseudo_p): Likewise.
> * sel-sched-ir.c (merge_fences): Likewise.
>
> libcpp/ChangeLog:
>
> PR c/80076
> * include/line-map.h (first_map_in_common): Declare.
> * line-map.c (first_map_in_common): Remove static.
>
> gcc/testsuite/ChangeLog:
>
> PR c/80076
> * c-c++-common/Wmisleading-indentation-pr80076.c: New test.
Ping.
> ---
> gcc/c-family/c-indentation.c | 61 ++++++++--
> gcc/gensupport.c | 2 +-
> gcc/lra-constraints.c | 12 +-
> gcc/objc/objc-gnu-runtime-abi-01.c | 4 +-
> gcc/objc/objc-next-runtime-abi-01.c | 4 +-
> gcc/sel-sched-ir.c | 112 +++++++++---------
> .../c-c++-common/Wmisleading-indentation-5.c | 56 +++++++++
> libcpp/include/line-map.h | 6 +
> libcpp/line-map.c | 2 +-
> 9 files changed, 178 insertions(+), 81 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
>
> diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
> index d814f6f29e6..8b88a8adc7c 100644
> --- a/gcc/c-family/c-indentation.c
> +++ b/gcc/c-family/c-indentation.c
> @@ -213,19 +213,6 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> const token_indent_info &body_tinfo,
> const token_indent_info &next_tinfo)
> {
> - location_t guard_loc = guard_tinfo.location;
> - location_t body_loc = body_tinfo.location;
> - location_t next_stmt_loc = next_tinfo.location;
> -
> - enum cpp_ttype body_type = body_tinfo.type;
> - enum cpp_ttype next_tok_type = next_tinfo.type;
> -
> - /* Don't attempt to compare the indentation of BODY_LOC and NEXT_STMT_LOC
> - if either are within macros. */
> - if (linemap_location_from_macro_expansion_p (line_table, body_loc)
> - || linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
> - return false;
> -
> /* Don't attempt to compare indentation if #line or # 44 "file"-style
> directives are present, suggesting generated code.
>
> @@ -266,6 +253,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> } <- NEXT
> baz ();
> */
> + enum cpp_ttype next_tok_type = next_tinfo.type;
> if (next_tok_type == CPP_CLOSE_BRACE
> || next_tinfo.keyword == RID_ELSE)
> return false;
> @@ -287,6 +275,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> bar (); <- BODY
> baz (); <- NEXT
> */
> + enum cpp_ttype body_type = body_tinfo.type;
> if (body_type == CPP_OPEN_BRACE)
> return false;
>
> @@ -294,6 +283,52 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> if (next_tok_type == CPP_SEMICOLON)
> return false;
>
> + location_t guard_loc = guard_tinfo.location;
> + location_t body_loc = body_tinfo.location;
> + location_t next_stmt_loc = next_tinfo.location;
> +
> + /* Resolve each token location to the respective macro expansion
> + point that produced the token. */
> + if (linemap_location_from_macro_expansion_p (line_table, guard_loc))
> + guard_loc = linemap_resolve_location (line_table, guard_loc,
> + LRK_MACRO_EXPANSION_POINT, NULL);
> + if (linemap_location_from_macro_expansion_p (line_table, body_loc))
> + body_loc = linemap_resolve_location (line_table, body_loc,
> + LRK_MACRO_EXPANSION_POINT, NULL);
> + if (linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
> + next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
> + LRK_MACRO_EXPANSION_POINT, NULL);
> +
> + /* When all three tokens are produced from a single macro expansion, we
> + instead consider their loci inside that macro's definition. */
> + if (guard_loc == body_loc && body_loc == next_stmt_loc)
> + {
> + const line_map *guard_body_common_map
> + = first_map_in_common (line_table,
> + guard_tinfo.location, body_tinfo.location,
> + &guard_loc, &body_loc);
> + const line_map *body_next_common_map
> + = first_map_in_common (line_table,
> + body_tinfo.location, next_tinfo.location,
> + &body_loc, &next_stmt_loc);
> +
> + /* Punt on complicated nesting of macros. */
> + if (guard_body_common_map != body_next_common_map)
> + return false;
> +
> + guard_loc = linemap_resolve_location (line_table, guard_loc,
> + LRK_MACRO_DEFINITION_LOCATION, NULL);
> + body_loc = linemap_resolve_location (line_table, body_loc,
> + LRK_MACRO_DEFINITION_LOCATION, NULL);
> + next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
> + LRK_MACRO_DEFINITION_LOCATION,
> + NULL);
> + }
> +
> + /* Give up if the loci are not all distinct. */
> + if (guard_loc == body_loc || body_loc == next_stmt_loc)
> + return false;
> +
> expanded_location body_exploc = expand_location (body_loc);
> expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
> expanded_location guard_exploc = expand_location (guard_loc);
> diff --git a/gcc/gensupport.c b/gcc/gensupport.c
> index f2ad54f0c55..61691aadff1 100644
> --- a/gcc/gensupport.c
> +++ b/gcc/gensupport.c
> @@ -1501,7 +1501,7 @@ alter_attrs_for_subst_insn (class queue_elem * elem, int n_dup)
> case SET_ATTR:
> if (strchr (XSTR (sub, 1), ',') != NULL)
> XSTR (sub, 1) = duplicate_alternatives (XSTR (sub, 1), n_dup);
> - break;
> + break;
>
> case SET_ATTR_ALTERNATIVE:
> case SET:
> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
> index 421c453997b..90ca31632f1 100644
> --- a/gcc/lra-constraints.c
> +++ b/gcc/lra-constraints.c
> @@ -4707,12 +4707,12 @@ multi_block_pseudo_p (int regno)
> if (regno < FIRST_PSEUDO_REGISTER)
> return false;
>
> - EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
> - if (bb == NULL)
> - bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
> - else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
> - return true;
> - return false;
> + EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
> + if (bb == NULL)
> + bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
> + else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
> + return true;
> + return false;
> }
>
> /* Return true if LIST contains a deleted insn. */
> diff --git a/gcc/objc/objc-gnu-runtime-abi-01.c b/gcc/objc/objc-gnu-runtime-abi-01.c
> index d5862435c29..c9959a7e1e8 100644
> --- a/gcc/objc/objc-gnu-runtime-abi-01.c
> +++ b/gcc/objc/objc-gnu-runtime-abi-01.c
> @@ -821,7 +821,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> ucls_super_ref =
> objc_build_component_ref (imp->class_decl,
> get_identifier ("super_class"));
> - return ucls_super_ref;
> + return ucls_super_ref;
> }
> else
> {
> @@ -829,7 +829,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> uucls_super_ref =
> objc_build_component_ref (imp->meta_decl,
> get_identifier ("super_class"));
> - return uucls_super_ref;
> + return uucls_super_ref;
> }
> }
>
> diff --git a/gcc/objc/objc-next-runtime-abi-01.c b/gcc/objc/objc-next-runtime-abi-01.c
> index 5c34fcb05cb..233d89e75b5 100644
> --- a/gcc/objc/objc-next-runtime-abi-01.c
> +++ b/gcc/objc/objc-next-runtime-abi-01.c
> @@ -938,7 +938,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> ucls_super_ref =
> objc_build_component_ref (imp->class_decl,
> get_identifier ("super_class"));
> - return ucls_super_ref;
> + return ucls_super_ref;
> }
> else
> {
> @@ -946,7 +946,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> uucls_super_ref =
> objc_build_component_ref (imp->meta_decl,
> get_identifier ("super_class"));
> - return uucls_super_ref;
> + return uucls_super_ref;
> }
> }
>
> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> index 4f1e4afdc4c..1dd981ff8d5 100644
> --- a/gcc/sel-sched-ir.c
> +++ b/gcc/sel-sched-ir.c
> @@ -722,63 +722,63 @@ merge_fences (fence_t f, insn_t insn,
> != BLOCK_FOR_INSN (last_scheduled_insn));
> }
>
> - /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
> - FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
> - SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> - {
> - if (succ == insn)
> - {
> - /* No same successor allowed from several edges. */
> - gcc_assert (!edge_old);
> - edge_old = si.e1;
> - }
> - }
> - /* Find edge of second predecessor (last_scheduled_insn->insn). */
> - FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
> - SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> - {
> - if (succ == insn)
> - {
> - /* No same successor allowed from several edges. */
> - gcc_assert (!edge_new);
> - edge_new = si.e1;
> - }
> - }
> + /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
> + FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
> + SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> + {
> + if (succ == insn)
> + {
> + /* No same successor allowed from several edges. */
> + gcc_assert (!edge_old);
> + edge_old = si.e1;
> + }
> + }
> + /* Find edge of second predecessor (last_scheduled_insn->insn). */
> + FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
> + SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> + {
> + if (succ == insn)
> + {
> + /* No same successor allowed from several edges. */
> + gcc_assert (!edge_new);
> + edge_new = si.e1;
> + }
> + }
>
> - /* Check if we can choose most probable predecessor. */
> - if (edge_old == NULL || edge_new == NULL)
> - {
> - reset_deps_context (FENCE_DC (f));
> - delete_deps_context (dc);
> - vec_free (executing_insns);
> - free (ready_ticks);
> -
> - FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
> - if (FENCE_EXECUTING_INSNS (f))
> - FENCE_EXECUTING_INSNS (f)->block_remove (0,
> - FENCE_EXECUTING_INSNS (f)->length ());
> - if (FENCE_READY_TICKS (f))
> - memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
> - }
> - else
> - if (edge_new->probability > edge_old->probability)
> - {
> - delete_deps_context (FENCE_DC (f));
> - FENCE_DC (f) = dc;
> - vec_free (FENCE_EXECUTING_INSNS (f));
> - FENCE_EXECUTING_INSNS (f) = executing_insns;
> - free (FENCE_READY_TICKS (f));
> - FENCE_READY_TICKS (f) = ready_ticks;
> - FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
> - FENCE_CYCLE (f) = cycle;
> - }
> - else
> - {
> - /* Leave DC and CYCLE untouched. */
> - delete_deps_context (dc);
> - vec_free (executing_insns);
> - free (ready_ticks);
> - }
> + /* Check if we can choose most probable predecessor. */
> + if (edge_old == NULL || edge_new == NULL)
> + {
> + reset_deps_context (FENCE_DC (f));
> + delete_deps_context (dc);
> + vec_free (executing_insns);
> + free (ready_ticks);
> +
> + FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
> + if (FENCE_EXECUTING_INSNS (f))
> + FENCE_EXECUTING_INSNS (f)->block_remove (0,
> + FENCE_EXECUTING_INSNS (f)->length ());
> + if (FENCE_READY_TICKS (f))
> + memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
> + }
> + else
> + if (edge_new->probability > edge_old->probability)
> + {
> + delete_deps_context (FENCE_DC (f));
> + FENCE_DC (f) = dc;
> + vec_free (FENCE_EXECUTING_INSNS (f));
> + FENCE_EXECUTING_INSNS (f) = executing_insns;
> + free (FENCE_READY_TICKS (f));
> + FENCE_READY_TICKS (f) = ready_ticks;
> + FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
> + FENCE_CYCLE (f) = cycle;
> + }
> + else
> + {
> + /* Leave DC and CYCLE untouched. */
> + delete_deps_context (dc);
> + vec_free (executing_insns);
> + free (ready_ticks);
> + }
> }
>
> /* Fill remaining invariant fields. */
> diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
> new file mode 100644
> index 00000000000..12b53569ba7
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
> @@ -0,0 +1,56 @@
> +/* PR c/80076 */
> +/* { dg-options "-Wmisleading-indentation" } */
> +
> +void foo(void);
> +
> +void test01(int flag) {
> +#define bar() foo() /* { dg-message "this statement" } */
> + if (flag) /* { dg-warning "does not guard" } */
> + foo();
> + bar(); /* { dg-message "in expansion of macro" } */
> +#undef bar
> +}
> +
> +void test02(int flag) {
> +#define bar() foo()
> + if (flag) /* { dg-warning "does not guard" } */
> + bar();
> + foo(); /* { dg-message "this statement" } */
> +#undef bar
> +}
> +
> +void test03(int flag) {
> +#define bar() foo() /* { dg-message "this statement" } */
> + if (flag) /* { dg-warning "does not guard" } */
> + bar();
> + bar(); /* { dg-message "in expansion of macro" } */
> +#undef bar
> +}
> +
> +void test04(int flag, int num) {
> +#define bar() \
> + { \
> + if (flag) \
> + num = 0; \
> + num = 1; \
> + }
> + bar();
> +/* { dg-warning "does not guard" "" { target *-*-* } .-5 } */
> +/* { dg-message "this statement" "" { target *-*-* } .-4 } */
> +#undef bar
> +}
> +
> +void test05(int flag, int num) {
> +#define baz() (num = 1)
> +#define bar() \
> + { \
> + if (flag) \
> + num = 0; \
> + baz(); \
> + }
> +#define wrapper bar
> + wrapper();
> +/* { dg-warning "does not guard" "" { target *-*-* } .-6 } */
> +/* { dg-message "this statement" "" { target *-*-* } .-10 } */
> +#undef bar
> +}
> diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> index 217f916fc35..44008be5c08 100644
> --- a/libcpp/include/line-map.h
> +++ b/libcpp/include/line-map.h
> @@ -1225,6 +1225,12 @@ LINEMAP_SYSP (const line_map_ordinary *ord_map)
> return ord_map->sysp;
> }
>
> +const struct line_map *first_map_in_common (line_maps *set,
> + location_t loc0,
> + location_t loc1,
> + location_t *res_loc0,
> + location_t *res_loc1);
> +
> /* Return a positive value if PRE denotes the location of a token that
> comes before the token of POST, 0 if PRE denotes the location of
> the same token as the token for POST, and a negative value
> diff --git a/libcpp/line-map.c b/libcpp/line-map.c
> index a8d52861dee..5a74174579f 100644
> --- a/libcpp/line-map.c
> +++ b/libcpp/line-map.c
> @@ -1289,7 +1289,7 @@ first_map_in_common_1 (line_maps *set,
> virtual location of the token inside the resulting macro, upon
> return of a non-NULL result. */
>
> -static const struct line_map*
> +const struct line_map*
> first_map_in_common (line_maps *set,
> location_t loc0,
> location_t loc1,
> --
> 2.28.0.rc1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076]
2020-08-10 13:46 ` Patrick Palka
@ 2020-09-10 13:40 ` Patrick Palka
0 siblings, 0 replies; 8+ messages in thread
From: Patrick Palka @ 2020-09-10 13:40 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches, dmalcolm
On Mon, 10 Aug 2020, Patrick Palka wrote:
> On Tue, 28 Jul 2020, Patrick Palka wrote:
>
> > Currently the -Wmisleading-indentation warning doesn't do any analysis
> > when the guarded statement or the statement after it is produced by a
> > macro, as the mentioned PR illustrates. This means that we warn for:
> >
> > if (flag)
> > foo ();
> > bar ();
> >
> > and yet we don't warn for:
> >
> > #define BAR bar
> > if (flag)
> > foo ();
> > BAR ();
> >
> > which seems like a surprising limitation.
> >
> > This patch extends the -Wmisleading-indentation implementation to
> > support analyzing such statements and their tokens. This is done in the
> > "natural" way by resolving the location of each of the three tokens to
> > the token's macro expansion point. (Additionally, if the tokens all
> > resolve to the same macro expansion point then we instead use their
> > locations within the macro definition.) When these resolved locations
> > are all different, then we can proceed with applying the warning
> > heuristics to them as if no macros were involved.
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu,
> > powerpc64le-unknown-linux-gnu, aarch64-unknown-linux-gnu and
> > s390x-ibm-linux-gnu.
> >
> > I also built sqlite3, glibc, linux, and a number of other projects with
> > this patch, to help verify that it doesn't introduce bogus warnings.
> > All new warnings that I've seen look justified from what I can tell,
> > e.g. we now warn about the following line, which is indented as if it's
> > guarded by the if statement two lines above it:
> > https://github.com/torvalds/linux/blob/fe557319aa06c23cffc9346000f119547e0f289a/drivers/misc/kgdbts.c#L105
> >
> > Does this look OK to commit?
> >
> > gcc/c-family/ChangeLog:
> >
> > PR c/80076
> > * c-indentation.c (should_warn_for_misleading_indentation): Move
> > declarations of local variables closer to their first use.
> > Handle virtual token locations by resolving them to their
> > respective macro expansion points. If all three tokens are
> > produced from the same macro expansion, then instead use their
> > loci within the macro definition.
> >
> > gcc/objc/ChangeLog:
> >
> > PR c/80076
> > * objc-gnu-runtime-abi-01.c
> > (gnu_runtime_abi_01_get_class_super_ref): Reduce indentation of
> > misleadingly indented return statements.
> > * objc-next-runtime-abi-01.c
> > (next_runtime_abi_01_get_class_super_ref): Likewise.
> >
> > gcc/ChangeLog:
> >
> > PR c/80076
> > * gensupport.c (alter_attrs_for_subst_insn) <case SET_ATTR>:
> > Reduce indentation of misleadingly indented code fragment.
> > * lra-constraints.c (multi_block_pseudo_p): Likewise.
> > * sel-sched-ir.c (merge_fences): Likewise.
> >
> > libcpp/ChangeLog:
> >
> > PR c/80076
> > * include/line-map.h (first_map_in_common): Declare.
> > * line-map.c (first_map_in_common): Remove static.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c/80076
> > * c-c++-common/Wmisleading-indentation-pr80076.c: New test.
>
> Ping.
Ping^2
>
> > ---
> > gcc/c-family/c-indentation.c | 61 ++++++++--
> > gcc/gensupport.c | 2 +-
> > gcc/lra-constraints.c | 12 +-
> > gcc/objc/objc-gnu-runtime-abi-01.c | 4 +-
> > gcc/objc/objc-next-runtime-abi-01.c | 4 +-
> > gcc/sel-sched-ir.c | 112 +++++++++---------
> > .../c-c++-common/Wmisleading-indentation-5.c | 56 +++++++++
> > libcpp/include/line-map.h | 6 +
> > libcpp/line-map.c | 2 +-
> > 9 files changed, 178 insertions(+), 81 deletions(-)
> > create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
> >
> > diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
> > index d814f6f29e6..8b88a8adc7c 100644
> > --- a/gcc/c-family/c-indentation.c
> > +++ b/gcc/c-family/c-indentation.c
> > @@ -213,19 +213,6 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> > const token_indent_info &body_tinfo,
> > const token_indent_info &next_tinfo)
> > {
> > - location_t guard_loc = guard_tinfo.location;
> > - location_t body_loc = body_tinfo.location;
> > - location_t next_stmt_loc = next_tinfo.location;
> > -
> > - enum cpp_ttype body_type = body_tinfo.type;
> > - enum cpp_ttype next_tok_type = next_tinfo.type;
> > -
> > - /* Don't attempt to compare the indentation of BODY_LOC and NEXT_STMT_LOC
> > - if either are within macros. */
> > - if (linemap_location_from_macro_expansion_p (line_table, body_loc)
> > - || linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
> > - return false;
> > -
> > /* Don't attempt to compare indentation if #line or # 44 "file"-style
> > directives are present, suggesting generated code.
> >
> > @@ -266,6 +253,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> > } <- NEXT
> > baz ();
> > */
> > + enum cpp_ttype next_tok_type = next_tinfo.type;
> > if (next_tok_type == CPP_CLOSE_BRACE
> > || next_tinfo.keyword == RID_ELSE)
> > return false;
> > @@ -287,6 +275,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> > bar (); <- BODY
> > baz (); <- NEXT
> > */
> > + enum cpp_ttype body_type = body_tinfo.type;
> > if (body_type == CPP_OPEN_BRACE)
> > return false;
> >
> > @@ -294,6 +283,52 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> > if (next_tok_type == CPP_SEMICOLON)
> > return false;
> >
> > + location_t guard_loc = guard_tinfo.location;
> > + location_t body_loc = body_tinfo.location;
> > + location_t next_stmt_loc = next_tinfo.location;
> > +
> > + /* Resolve each token location to the respective macro expansion
> > + point that produced the token. */
> > + if (linemap_location_from_macro_expansion_p (line_table, guard_loc))
> > + guard_loc = linemap_resolve_location (line_table, guard_loc,
> > + LRK_MACRO_EXPANSION_POINT, NULL);
> > + if (linemap_location_from_macro_expansion_p (line_table, body_loc))
> > + body_loc = linemap_resolve_location (line_table, body_loc,
> > + LRK_MACRO_EXPANSION_POINT, NULL);
> > + if (linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
> > + next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
> > + LRK_MACRO_EXPANSION_POINT, NULL);
> > +
> > + /* When all three tokens are produced from a single macro expansion, we
> > + instead consider their loci inside that macro's definition. */
> > + if (guard_loc == body_loc && body_loc == next_stmt_loc)
> > + {
> > + const line_map *guard_body_common_map
> > + = first_map_in_common (line_table,
> > + guard_tinfo.location, body_tinfo.location,
> > + &guard_loc, &body_loc);
> > + const line_map *body_next_common_map
> > + = first_map_in_common (line_table,
> > + body_tinfo.location, next_tinfo.location,
> > + &body_loc, &next_stmt_loc);
> > +
> > + /* Punt on complicated nesting of macros. */
> > + if (guard_body_common_map != body_next_common_map)
> > + return false;
> > +
> > + guard_loc = linemap_resolve_location (line_table, guard_loc,
> > + LRK_MACRO_DEFINITION_LOCATION, NULL);
> > + body_loc = linemap_resolve_location (line_table, body_loc,
> > + LRK_MACRO_DEFINITION_LOCATION, NULL);
> > + next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
> > + LRK_MACRO_DEFINITION_LOCATION,
> > + NULL);
> > + }
> > +
> > + /* Give up if the loci are not all distinct. */
> > + if (guard_loc == body_loc || body_loc == next_stmt_loc)
> > + return false;
> > +
> > expanded_location body_exploc = expand_location (body_loc);
> > expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
> > expanded_location guard_exploc = expand_location (guard_loc);
> > diff --git a/gcc/gensupport.c b/gcc/gensupport.c
> > index f2ad54f0c55..61691aadff1 100644
> > --- a/gcc/gensupport.c
> > +++ b/gcc/gensupport.c
> > @@ -1501,7 +1501,7 @@ alter_attrs_for_subst_insn (class queue_elem * elem, int n_dup)
> > case SET_ATTR:
> > if (strchr (XSTR (sub, 1), ',') != NULL)
> > XSTR (sub, 1) = duplicate_alternatives (XSTR (sub, 1), n_dup);
> > - break;
> > + break;
> >
> > case SET_ATTR_ALTERNATIVE:
> > case SET:
> > diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
> > index 421c453997b..90ca31632f1 100644
> > --- a/gcc/lra-constraints.c
> > +++ b/gcc/lra-constraints.c
> > @@ -4707,12 +4707,12 @@ multi_block_pseudo_p (int regno)
> > if (regno < FIRST_PSEUDO_REGISTER)
> > return false;
> >
> > - EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
> > - if (bb == NULL)
> > - bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
> > - else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
> > - return true;
> > - return false;
> > + EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
> > + if (bb == NULL)
> > + bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
> > + else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
> > + return true;
> > + return false;
> > }
> >
> > /* Return true if LIST contains a deleted insn. */
> > diff --git a/gcc/objc/objc-gnu-runtime-abi-01.c b/gcc/objc/objc-gnu-runtime-abi-01.c
> > index d5862435c29..c9959a7e1e8 100644
> > --- a/gcc/objc/objc-gnu-runtime-abi-01.c
> > +++ b/gcc/objc/objc-gnu-runtime-abi-01.c
> > @@ -821,7 +821,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> > ucls_super_ref =
> > objc_build_component_ref (imp->class_decl,
> > get_identifier ("super_class"));
> > - return ucls_super_ref;
> > + return ucls_super_ref;
> > }
> > else
> > {
> > @@ -829,7 +829,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> > uucls_super_ref =
> > objc_build_component_ref (imp->meta_decl,
> > get_identifier ("super_class"));
> > - return uucls_super_ref;
> > + return uucls_super_ref;
> > }
> > }
> >
> > diff --git a/gcc/objc/objc-next-runtime-abi-01.c b/gcc/objc/objc-next-runtime-abi-01.c
> > index 5c34fcb05cb..233d89e75b5 100644
> > --- a/gcc/objc/objc-next-runtime-abi-01.c
> > +++ b/gcc/objc/objc-next-runtime-abi-01.c
> > @@ -938,7 +938,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> > ucls_super_ref =
> > objc_build_component_ref (imp->class_decl,
> > get_identifier ("super_class"));
> > - return ucls_super_ref;
> > + return ucls_super_ref;
> > }
> > else
> > {
> > @@ -946,7 +946,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> > uucls_super_ref =
> > objc_build_component_ref (imp->meta_decl,
> > get_identifier ("super_class"));
> > - return uucls_super_ref;
> > + return uucls_super_ref;
> > }
> > }
> >
> > diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> > index 4f1e4afdc4c..1dd981ff8d5 100644
> > --- a/gcc/sel-sched-ir.c
> > +++ b/gcc/sel-sched-ir.c
> > @@ -722,63 +722,63 @@ merge_fences (fence_t f, insn_t insn,
> > != BLOCK_FOR_INSN (last_scheduled_insn));
> > }
> >
> > - /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
> > - FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
> > - SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> > - {
> > - if (succ == insn)
> > - {
> > - /* No same successor allowed from several edges. */
> > - gcc_assert (!edge_old);
> > - edge_old = si.e1;
> > - }
> > - }
> > - /* Find edge of second predecessor (last_scheduled_insn->insn). */
> > - FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
> > - SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> > - {
> > - if (succ == insn)
> > - {
> > - /* No same successor allowed from several edges. */
> > - gcc_assert (!edge_new);
> > - edge_new = si.e1;
> > - }
> > - }
> > + /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
> > + FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
> > + SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> > + {
> > + if (succ == insn)
> > + {
> > + /* No same successor allowed from several edges. */
> > + gcc_assert (!edge_old);
> > + edge_old = si.e1;
> > + }
> > + }
> > + /* Find edge of second predecessor (last_scheduled_insn->insn). */
> > + FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
> > + SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> > + {
> > + if (succ == insn)
> > + {
> > + /* No same successor allowed from several edges. */
> > + gcc_assert (!edge_new);
> > + edge_new = si.e1;
> > + }
> > + }
> >
> > - /* Check if we can choose most probable predecessor. */
> > - if (edge_old == NULL || edge_new == NULL)
> > - {
> > - reset_deps_context (FENCE_DC (f));
> > - delete_deps_context (dc);
> > - vec_free (executing_insns);
> > - free (ready_ticks);
> > -
> > - FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
> > - if (FENCE_EXECUTING_INSNS (f))
> > - FENCE_EXECUTING_INSNS (f)->block_remove (0,
> > - FENCE_EXECUTING_INSNS (f)->length ());
> > - if (FENCE_READY_TICKS (f))
> > - memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
> > - }
> > - else
> > - if (edge_new->probability > edge_old->probability)
> > - {
> > - delete_deps_context (FENCE_DC (f));
> > - FENCE_DC (f) = dc;
> > - vec_free (FENCE_EXECUTING_INSNS (f));
> > - FENCE_EXECUTING_INSNS (f) = executing_insns;
> > - free (FENCE_READY_TICKS (f));
> > - FENCE_READY_TICKS (f) = ready_ticks;
> > - FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
> > - FENCE_CYCLE (f) = cycle;
> > - }
> > - else
> > - {
> > - /* Leave DC and CYCLE untouched. */
> > - delete_deps_context (dc);
> > - vec_free (executing_insns);
> > - free (ready_ticks);
> > - }
> > + /* Check if we can choose most probable predecessor. */
> > + if (edge_old == NULL || edge_new == NULL)
> > + {
> > + reset_deps_context (FENCE_DC (f));
> > + delete_deps_context (dc);
> > + vec_free (executing_insns);
> > + free (ready_ticks);
> > +
> > + FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
> > + if (FENCE_EXECUTING_INSNS (f))
> > + FENCE_EXECUTING_INSNS (f)->block_remove (0,
> > + FENCE_EXECUTING_INSNS (f)->length ());
> > + if (FENCE_READY_TICKS (f))
> > + memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
> > + }
> > + else
> > + if (edge_new->probability > edge_old->probability)
> > + {
> > + delete_deps_context (FENCE_DC (f));
> > + FENCE_DC (f) = dc;
> > + vec_free (FENCE_EXECUTING_INSNS (f));
> > + FENCE_EXECUTING_INSNS (f) = executing_insns;
> > + free (FENCE_READY_TICKS (f));
> > + FENCE_READY_TICKS (f) = ready_ticks;
> > + FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
> > + FENCE_CYCLE (f) = cycle;
> > + }
> > + else
> > + {
> > + /* Leave DC and CYCLE untouched. */
> > + delete_deps_context (dc);
> > + vec_free (executing_insns);
> > + free (ready_ticks);
> > + }
> > }
> >
> > /* Fill remaining invariant fields. */
> > diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
> > new file mode 100644
> > index 00000000000..12b53569ba7
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
> > @@ -0,0 +1,56 @@
> > +/* PR c/80076 */
> > +/* { dg-options "-Wmisleading-indentation" } */
> > +
> > +void foo(void);
> > +
> > +void test01(int flag) {
> > +#define bar() foo() /* { dg-message "this statement" } */
> > + if (flag) /* { dg-warning "does not guard" } */
> > + foo();
> > + bar(); /* { dg-message "in expansion of macro" } */
> > +#undef bar
> > +}
> > +
> > +void test02(int flag) {
> > +#define bar() foo()
> > + if (flag) /* { dg-warning "does not guard" } */
> > + bar();
> > + foo(); /* { dg-message "this statement" } */
> > +#undef bar
> > +}
> > +
> > +void test03(int flag) {
> > +#define bar() foo() /* { dg-message "this statement" } */
> > + if (flag) /* { dg-warning "does not guard" } */
> > + bar();
> > + bar(); /* { dg-message "in expansion of macro" } */
> > +#undef bar
> > +}
> > +
> > +void test04(int flag, int num) {
> > +#define bar() \
> > + { \
> > + if (flag) \
> > + num = 0; \
> > + num = 1; \
> > + }
> > + bar();
> > +/* { dg-warning "does not guard" "" { target *-*-* } .-5 } */
> > +/* { dg-message "this statement" "" { target *-*-* } .-4 } */
> > +#undef bar
> > +}
> > +
> > +void test05(int flag, int num) {
> > +#define baz() (num = 1)
> > +#define bar() \
> > + { \
> > + if (flag) \
> > + num = 0; \
> > + baz(); \
> > + }
> > +#define wrapper bar
> > + wrapper();
> > +/* { dg-warning "does not guard" "" { target *-*-* } .-6 } */
> > +/* { dg-message "this statement" "" { target *-*-* } .-10 } */
> > +#undef bar
> > +}
> > diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> > index 217f916fc35..44008be5c08 100644
> > --- a/libcpp/include/line-map.h
> > +++ b/libcpp/include/line-map.h
> > @@ -1225,6 +1225,12 @@ LINEMAP_SYSP (const line_map_ordinary *ord_map)
> > return ord_map->sysp;
> > }
> >
> > +const struct line_map *first_map_in_common (line_maps *set,
> > + location_t loc0,
> > + location_t loc1,
> > + location_t *res_loc0,
> > + location_t *res_loc1);
> > +
> > /* Return a positive value if PRE denotes the location of a token that
> > comes before the token of POST, 0 if PRE denotes the location of
> > the same token as the token for POST, and a negative value
> > diff --git a/libcpp/line-map.c b/libcpp/line-map.c
> > index a8d52861dee..5a74174579f 100644
> > --- a/libcpp/line-map.c
> > +++ b/libcpp/line-map.c
> > @@ -1289,7 +1289,7 @@ first_map_in_common_1 (line_maps *set,
> > virtual location of the token inside the resulting macro, upon
> > return of a non-NULL result. */
> >
> > -static const struct line_map*
> > +const struct line_map*
> > first_map_in_common (line_maps *set,
> > location_t loc0,
> > location_t loc1,
> > --
> > 2.28.0.rc1
> >
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076]
2020-07-29 0:22 ` Patrick Palka
@ 2020-09-10 17:01 ` David Malcolm
2020-09-10 19:53 ` Patrick Palka
0 siblings, 1 reply; 8+ messages in thread
From: David Malcolm @ 2020-09-10 17:01 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches
On Tue, 2020-07-28 at 20:22 -0400, Patrick Palka wrote:
> On Tue, 28 Jul 2020, David Malcolm wrote:
>
> > On Tue, 2020-07-28 at 15:50 -0400, Patrick Palka wrote:
> > > Currently the -Wmisleading-indentation warning doesn't do any
> > > analysis
> > > when the guarded statement or the statement after it is produced
> > > by a
> > > macro, as the mentioned PR illustrates. This means that we warn
> > > for:
> > >
> > > if (flag)
> > > foo ();
> > > bar ();
> > >
> > > and yet we don't warn for:
> > >
> > > #define BAR bar
> > > if (flag)
> > > foo ();
> > > BAR ();
> > >
> > > which seems like a surprising limitation.
> >
> > IIRC we were trying to keep things simple in the initial
> > implementation.
> >
> > > This patch extends the -Wmisleading-indentation implementation to
> > > support analyzing such statements and their tokens. This is done
> > > in
> > > the
> > > "natural" way by resolving the location of each of the three
> > > tokens
> > > to
> > > the token's macro expansion point. (Additionally, if the tokens
> > > all
> > > resolve to the same macro expansion point then we instead use
> > > their
> > > locations within the macro definition.) When these resolved
> > > locations
> > > are all different, then we can proceed with applying the warning
> > > heuristics to them as if no macros were involved.
> >
> > Thanks for working on this. I've only looked at the patch briefly,
> > but
> > so far it looks reasonable.
> >
> > Can you post some examples of what the output looks like for some
> > of
> > these cases? The diagnostics code has some logic for printing how
> > macros are unwound, so I'm wondering what we actually print for
> > these
> > cases, and if it looks intelligble to an end-user.
>
> For the code fragment mentioned above:
>
> int
> main()
> {
> #define BAR
> if (flag)
> foo ();
> BAR ();
> }
>
> we emit:
>
> test.cc: In function ‘int main()’:
> test.cc:7:3: warning: this ‘if’ clause does not guard... [-
> Wmisleading-indentation]
> 9 | if (flag)
> | ^~
> test.cc:8:15: note: ...this statement, but the latter is misleadingly
> indented as if it were guarded by the ‘if’
> 8 | #define BAR bar
> | ^~~
> test.cc:10:5: note: in expansion of macro ‘BAR’
> 11 | BAR ();
> | ^~~
>
>
> And when all tokens are generated from the same macro, e.g. for:
>
> int
> main()
> {
> #define BAR bar
> #define DO_STUFF \
> if (flag) \
> foo (); \
> BAR ();
> DO_STUFF;
> }
>
> we emit:
>
> test.cc: In function ‘int main()’:
> test.cc:11:5: warning: this ‘if’ clause does not guard... [-
> Wmisleading-indentation]
> 11 | if (flag) \
> | ^~
> test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
> 14 | DO_STUFF;
> | ^~~~~~~~
> test.cc:9:15: note: ...this statement, but the latter is misleadingly
> indented as if it were guarded by the ‘if’
> 9 | #define BAR bar
> | ^~~
> test.cc:13:7: note: in expansion of macro ‘BAR’
> 13 | BAR ();
> | ^~~
> test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
> 14 | DO_STUFF;
> | ^~~~~~~~
>
> The automatic macro unwinding logic saves the day here and yields
> mostly
> legible output "for free". What do you think?
Sorry about the delay in responding.
I agree that the output is "mostly legible".
The patch is OK as-is, and looking at the fixes you made to our source
tree I think it's usefully catching problematic code.
That said, I wonder if the output could be improved for the macro case.
Within should_warn_for_misleading_indentation the patch can update
guard_loc, body_loc and next_stmt_loc before expand_location is called
on them, but then in warn_for_misleading_indentation it uses the
original guard_tinfo.location and next_tinfo.location for the locations
of the warning and note. I have a hunch that the readability of the
diagnostic would be improved for the macro case by updating those
locations whenever the inputs to expand_location are updated. It would
lose some detail from the point of view of the user grokking macro
expansions, but perhaps is better given the specific focus of this
warning on whitespace. Does doing so help for the various cases you've
tried?
Thanks
Dave
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076]
2020-09-10 17:01 ` David Malcolm
@ 2020-09-10 19:53 ` Patrick Palka
2020-09-17 18:30 ` Patrick Palka
0 siblings, 1 reply; 8+ messages in thread
From: Patrick Palka @ 2020-09-10 19:53 UTC (permalink / raw)
To: David Malcolm; +Cc: Patrick Palka, gcc-patches
On Thu, 10 Sep 2020, David Malcolm wrote:
> On Tue, 2020-07-28 at 20:22 -0400, Patrick Palka wrote:
> > On Tue, 28 Jul 2020, David Malcolm wrote:
> >
> > > On Tue, 2020-07-28 at 15:50 -0400, Patrick Palka wrote:
> > > > Currently the -Wmisleading-indentation warning doesn't do any
> > > > analysis
> > > > when the guarded statement or the statement after it is produced
> > > > by a
> > > > macro, as the mentioned PR illustrates. This means that we warn
> > > > for:
> > > >
> > > > if (flag)
> > > > foo ();
> > > > bar ();
> > > >
> > > > and yet we don't warn for:
> > > >
> > > > #define BAR bar
> > > > if (flag)
> > > > foo ();
> > > > BAR ();
> > > >
> > > > which seems like a surprising limitation.
> > >
> > > IIRC we were trying to keep things simple in the initial
> > > implementation.
> > >
> > > > This patch extends the -Wmisleading-indentation implementation to
> > > > support analyzing such statements and their tokens. This is done
> > > > in
> > > > the
> > > > "natural" way by resolving the location of each of the three
> > > > tokens
> > > > to
> > > > the token's macro expansion point. (Additionally, if the tokens
> > > > all
> > > > resolve to the same macro expansion point then we instead use
> > > > their
> > > > locations within the macro definition.) When these resolved
> > > > locations
> > > > are all different, then we can proceed with applying the warning
> > > > heuristics to them as if no macros were involved.
> > >
> > > Thanks for working on this. I've only looked at the patch briefly,
> > > but
> > > so far it looks reasonable.
> > >
> > > Can you post some examples of what the output looks like for some
> > > of
> > > these cases? The diagnostics code has some logic for printing how
> > > macros are unwound, so I'm wondering what we actually print for
> > > these
> > > cases, and if it looks intelligble to an end-user.
> >
> > For the code fragment mentioned above:
> >
> > int
> > main()
> > {
> > #define BAR
> > if (flag)
> > foo ();
> > BAR ();
> > }
> >
> > we emit:
> >
> > test.cc: In function ‘int main()’:
> > test.cc:7:3: warning: this ‘if’ clause does not guard... [-
> > Wmisleading-indentation]
> > 9 | if (flag)
> > | ^~
> > test.cc:8:15: note: ...this statement, but the latter is misleadingly
> > indented as if it were guarded by the ‘if’
> > 8 | #define BAR bar
> > | ^~~
> > test.cc:10:5: note: in expansion of macro ‘BAR’
> > 11 | BAR ();
> > | ^~~
> >
> >
> > And when all tokens are generated from the same macro, e.g. for:
> >
> > int
> > main()
> > {
> > #define BAR bar
> > #define DO_STUFF \
> > if (flag) \
> > foo (); \
> > BAR ();
> > DO_STUFF;
> > }
> >
> > we emit:
> >
> > test.cc: In function ‘int main()’:
> > test.cc:11:5: warning: this ‘if’ clause does not guard... [-
> > Wmisleading-indentation]
> > 11 | if (flag) \
> > | ^~
> > test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
> > 14 | DO_STUFF;
> > | ^~~~~~~~
> > test.cc:9:15: note: ...this statement, but the latter is misleadingly
> > indented as if it were guarded by the ‘if’
> > 9 | #define BAR bar
> > | ^~~
> > test.cc:13:7: note: in expansion of macro ‘BAR’
> > 13 | BAR ();
> > | ^~~
> > test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
> > 14 | DO_STUFF;
> > | ^~~~~~~~
> >
> > The automatic macro unwinding logic saves the day here and yields
> > mostly
> > legible output "for free". What do you think?
>
> Sorry about the delay in responding.
>
> I agree that the output is "mostly legible".
>
> The patch is OK as-is, and looking at the fixes you made to our source
> tree I think it's usefully catching problematic code.
>
Thanks for your review!
> That said, I wonder if the output could be improved for the macro case.
>
> Within should_warn_for_misleading_indentation the patch can update
> guard_loc, body_loc and next_stmt_loc before expand_location is called
> on them, but then in warn_for_misleading_indentation it uses the
> original guard_tinfo.location and next_tinfo.location for the locations
> of the warning and note. I have a hunch that the readability of the
> diagnostic would be improved for the macro case by updating those
> locations whenever the inputs to expand_location are updated. It would
> lose some detail from the point of view of the user grokking macro
> expansions, but perhaps is better given the specific focus of this
> warning on whitespace. Does doing so help for the various cases you've
> tried?
This makes a lot of sense to me and it seems to work well in the cases
I've tried.
As before, for the code fragment:
int
main()
{
#define BAR
if (flag)
foo ();
BAR ();
}
we now emit:
test.cc: In function ‘int main()’:
test.cc:7:3: warning: this ‘if’ clause does not guard... [-
Wmisleading-indentation]
9 | if (flag)
| ^~
test.cc:10:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
11 | BAR ();
| ^~~
When all tokens are generated from the same macro, e.g. for:
int
main()
{
#define BAR bar
#define DO_STUFF \
if (flag) \
foo (); \
BAR ();
DO_STUFF;
}
we emit... the same diagnostic as above :)
The below patch implements the approach you outlined; some existing
-Wmisleading-indentation testcases had to be adjusted accordingly. Is
something like this what you had in mind?
-- >8 --
gcc/c-family/ChangeLog:
PR c/80076
* c-indentation.c (should_warn_for_misleading_indentation): Take
each token_indent_info argument by reference. Move declarations
of local variables closer to their first use. Handle virtual
token locations by resolving them to their respective macro
expansion points. If all three tokens are produced from the
same macro expansion, then instead use their loci within the
macro definition. Update the token_indent_info::location of
each of the supplied arguments.
(warn_for_misleading_indentation): Take each token_indent_info
argument by value.
* c-indentation.h (warn_for_misleading_indentation): Adjust
declaration accordingly.
gcc/objc/ChangeLog:
PR c/80076
* objc-gnu-runtime-abi-01.c
(gnu_runtime_abi_01_get_class_super_ref): Reduce indentation of
misleadingly indented return statements.
* objc-next-runtime-abi-01.c
(next_runtime_abi_01_get_class_super_ref): Likewise.
gcc/ChangeLog:
PR c/80076
* gensupport.c (alter_attrs_for_subst_insn) <case SET_ATTR>:
Reduce indentation of misleadingly indented code fragment.
* lra-constraints.c (multi_block_pseudo_p): Likewise.
* sel-sched-ir.c (merge_fences): Likewise.
libcpp/ChangeLog:
PR c/80076
* include/line-map.h (first_map_in_common): Declare.
* line-map.c (first_map_in_common): Remove static.
gcc/testsuite/ChangeLog:
PR c/80076
* c-c++-common/Wmisleading-indentation.c: Don't expect a "in
expansion of macro ..." note when the guard token is inside a
macro. Expect the "this clause does not guard" warning at the
macro expansion point instead of at the macro definition point
when the guard token is inside a macro.
* c-c++-common/Wmisleading-indentation-3.c: Likewise.
* c-c++-common/Wmisleading-indentation-5.c: New test.
---
gcc/c-family/c-indentation.c | 80 ++++++++++---
gcc/c-family/c-indentation.h | 6 +-
gcc/gensupport.c | 2 +-
gcc/lra-constraints.c | 12 +-
gcc/objc/objc-gnu-runtime-abi-01.c | 4 +-
gcc/objc/objc-next-runtime-abi-01.c | 4 +-
gcc/sel-sched-ir.c | 112 +++++++++---------
.../c-c++-common/Wmisleading-indentation-3.c | 8 +-
.../c-c++-common/Wmisleading-indentation-5.c | 52 ++++++++
.../c-c++-common/Wmisleading-indentation.c | 16 +--
libcpp/include/line-map.h | 6 +
libcpp/line-map.c | 2 +-
12 files changed, 200 insertions(+), 104 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index d814f6f29e6..de7b426f35c 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -209,23 +209,10 @@ detect_intervening_unindent (const char *file,
description of that function below. */
static bool
-should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
- const token_indent_info &body_tinfo,
- const token_indent_info &next_tinfo)
+should_warn_for_misleading_indentation (token_indent_info &guard_tinfo,
+ token_indent_info &body_tinfo,
+ token_indent_info &next_tinfo)
{
- location_t guard_loc = guard_tinfo.location;
- location_t body_loc = body_tinfo.location;
- location_t next_stmt_loc = next_tinfo.location;
-
- enum cpp_ttype body_type = body_tinfo.type;
- enum cpp_ttype next_tok_type = next_tinfo.type;
-
- /* Don't attempt to compare the indentation of BODY_LOC and NEXT_STMT_LOC
- if either are within macros. */
- if (linemap_location_from_macro_expansion_p (line_table, body_loc)
- || linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
- return false;
-
/* Don't attempt to compare indentation if #line or # 44 "file"-style
directives are present, suggesting generated code.
@@ -266,6 +253,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
} <- NEXT
baz ();
*/
+ enum cpp_ttype next_tok_type = next_tinfo.type;
if (next_tok_type == CPP_CLOSE_BRACE
|| next_tinfo.keyword == RID_ELSE)
return false;
@@ -287,6 +275,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
bar (); <- BODY
baz (); <- NEXT
*/
+ enum cpp_ttype body_type = body_tinfo.type;
if (body_type == CPP_OPEN_BRACE)
return false;
@@ -294,6 +283,59 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
if (next_tok_type == CPP_SEMICOLON)
return false;
+ location_t guard_loc = guard_tinfo.location;
+ location_t body_loc = body_tinfo.location;
+ location_t next_stmt_loc = next_tinfo.location;
+
+ /* Resolve each token location to the respective macro expansion
+ point that produced the token. */
+ if (linemap_location_from_macro_expansion_p (line_table, guard_loc))
+ guard_loc = linemap_resolve_location (line_table, guard_loc,
+ LRK_MACRO_EXPANSION_POINT, NULL);
+ if (linemap_location_from_macro_expansion_p (line_table, body_loc))
+ body_loc = linemap_resolve_location (line_table, body_loc,
+ LRK_MACRO_EXPANSION_POINT, NULL);
+ if (linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
+ next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
+ LRK_MACRO_EXPANSION_POINT, NULL);
+
+ /* When all three tokens are produced from a single macro expansion, we
+ instead consider their loci inside that macro's definition. */
+ if (guard_loc == body_loc && body_loc == next_stmt_loc)
+ {
+ const line_map *guard_body_common_map
+ = first_map_in_common (line_table,
+ guard_tinfo.location, body_tinfo.location,
+ &guard_loc, &body_loc);
+ const line_map *body_next_common_map
+ = first_map_in_common (line_table,
+ body_tinfo.location, next_tinfo.location,
+ &body_loc, &next_stmt_loc);
+
+ /* Punt on complicated nesting of macros. */
+ if (guard_body_common_map != body_next_common_map)
+ return false;
+
+ guard_loc = linemap_resolve_location (line_table, guard_loc,
+ LRK_MACRO_DEFINITION_LOCATION, NULL);
+ body_loc = linemap_resolve_location (line_table, body_loc,
+ LRK_MACRO_DEFINITION_LOCATION, NULL);
+ next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
+ LRK_MACRO_DEFINITION_LOCATION,
+ NULL);
+ }
+
+ /* Give up if the loci are not all distinct. */
+ if (guard_loc == body_loc || body_loc == next_stmt_loc)
+ return false;
+
+ /* Now update the tokens' locations on the caller side too, so that the
+ diagnostics emitted by warn_for_misleading_indentation focus on the
+ problematic whitespace by omitting macro expansion notes. */
+ guard_tinfo.location = guard_loc;
+ body_tinfo.location = body_loc;
+ next_tinfo.location = next_stmt_loc;
+
expanded_location body_exploc = expand_location (body_loc);
expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
expanded_location guard_exploc = expand_location (guard_loc);
@@ -599,9 +641,9 @@ guard_tinfo_to_string (enum rid keyword)
GUARD_KIND identifies the kind of clause e.g. "if", "else" etc. */
void
-warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
- const token_indent_info &body_tinfo,
- const token_indent_info &next_tinfo)
+warn_for_misleading_indentation (token_indent_info guard_tinfo,
+ token_indent_info body_tinfo,
+ token_indent_info next_tinfo)
{
/* Early reject for the case where -Wmisleading-indentation is disabled,
to avoid doing work only to have the warning suppressed inside the
diff --git a/gcc/c-family/c-indentation.h b/gcc/c-family/c-indentation.h
index e183d3aafb7..67400f6d43b 100644
--- a/gcc/c-family/c-indentation.h
+++ b/gcc/c-family/c-indentation.h
@@ -45,9 +45,9 @@ get_token_indent_info (const T *token)
}
extern void
-warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
- const token_indent_info &body_tinfo,
- const token_indent_info &next_tinfo);
+warn_for_misleading_indentation (token_indent_info guard_tinfo,
+ token_indent_info body_tinfo,
+ token_indent_info next_tinfo);
extern const char *
guard_tinfo_to_string (enum rid keyword);
diff --git a/gcc/gensupport.c b/gcc/gensupport.c
index f2ad54f0c55..61691aadff1 100644
--- a/gcc/gensupport.c
+++ b/gcc/gensupport.c
@@ -1501,7 +1501,7 @@ alter_attrs_for_subst_insn (class queue_elem * elem, int n_dup)
case SET_ATTR:
if (strchr (XSTR (sub, 1), ',') != NULL)
XSTR (sub, 1) = duplicate_alternatives (XSTR (sub, 1), n_dup);
- break;
+ break;
case SET_ATTR_ALTERNATIVE:
case SET:
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 161b721efb1..301c912cb21 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -4776,12 +4776,12 @@ multi_block_pseudo_p (int regno)
if (regno < FIRST_PSEUDO_REGISTER)
return false;
- EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
- if (bb == NULL)
- bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
- else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
- return true;
- return false;
+ EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
+ if (bb == NULL)
+ bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
+ else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
+ return true;
+ return false;
}
/* Return true if LIST contains a deleted insn. */
diff --git a/gcc/objc/objc-gnu-runtime-abi-01.c b/gcc/objc/objc-gnu-runtime-abi-01.c
index d5862435c29..c9959a7e1e8 100644
--- a/gcc/objc/objc-gnu-runtime-abi-01.c
+++ b/gcc/objc/objc-gnu-runtime-abi-01.c
@@ -821,7 +821,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
ucls_super_ref =
objc_build_component_ref (imp->class_decl,
get_identifier ("super_class"));
- return ucls_super_ref;
+ return ucls_super_ref;
}
else
{
@@ -829,7 +829,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
uucls_super_ref =
objc_build_component_ref (imp->meta_decl,
get_identifier ("super_class"));
- return uucls_super_ref;
+ return uucls_super_ref;
}
}
diff --git a/gcc/objc/objc-next-runtime-abi-01.c b/gcc/objc/objc-next-runtime-abi-01.c
index 5c34fcb05cb..233d89e75b5 100644
--- a/gcc/objc/objc-next-runtime-abi-01.c
+++ b/gcc/objc/objc-next-runtime-abi-01.c
@@ -938,7 +938,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
ucls_super_ref =
objc_build_component_ref (imp->class_decl,
get_identifier ("super_class"));
- return ucls_super_ref;
+ return ucls_super_ref;
}
else
{
@@ -946,7 +946,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
uucls_super_ref =
objc_build_component_ref (imp->meta_decl,
get_identifier ("super_class"));
- return uucls_super_ref;
+ return uucls_super_ref;
}
}
diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index f58628ae92d..c8e086e4950 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -722,63 +722,63 @@ merge_fences (fence_t f, insn_t insn,
!= BLOCK_FOR_INSN (last_scheduled_insn));
}
- /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
- FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
- SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
- {
- if (succ == insn)
- {
- /* No same successor allowed from several edges. */
- gcc_assert (!edge_old);
- edge_old = si.e1;
- }
- }
- /* Find edge of second predecessor (last_scheduled_insn->insn). */
- FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
- SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
- {
- if (succ == insn)
- {
- /* No same successor allowed from several edges. */
- gcc_assert (!edge_new);
- edge_new = si.e1;
- }
- }
+ /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
+ FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
+ SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
+ {
+ if (succ == insn)
+ {
+ /* No same successor allowed from several edges. */
+ gcc_assert (!edge_old);
+ edge_old = si.e1;
+ }
+ }
+ /* Find edge of second predecessor (last_scheduled_insn->insn). */
+ FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
+ SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
+ {
+ if (succ == insn)
+ {
+ /* No same successor allowed from several edges. */
+ gcc_assert (!edge_new);
+ edge_new = si.e1;
+ }
+ }
- /* Check if we can choose most probable predecessor. */
- if (edge_old == NULL || edge_new == NULL)
- {
- reset_deps_context (FENCE_DC (f));
- delete_deps_context (dc);
- vec_free (executing_insns);
- free (ready_ticks);
-
- FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
- if (FENCE_EXECUTING_INSNS (f))
- FENCE_EXECUTING_INSNS (f)->block_remove (0,
- FENCE_EXECUTING_INSNS (f)->length ());
- if (FENCE_READY_TICKS (f))
- memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
- }
- else
- if (edge_new->probability > edge_old->probability)
- {
- delete_deps_context (FENCE_DC (f));
- FENCE_DC (f) = dc;
- vec_free (FENCE_EXECUTING_INSNS (f));
- FENCE_EXECUTING_INSNS (f) = executing_insns;
- free (FENCE_READY_TICKS (f));
- FENCE_READY_TICKS (f) = ready_ticks;
- FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
- FENCE_CYCLE (f) = cycle;
- }
- else
- {
- /* Leave DC and CYCLE untouched. */
- delete_deps_context (dc);
- vec_free (executing_insns);
- free (ready_ticks);
- }
+ /* Check if we can choose most probable predecessor. */
+ if (edge_old == NULL || edge_new == NULL)
+ {
+ reset_deps_context (FENCE_DC (f));
+ delete_deps_context (dc);
+ vec_free (executing_insns);
+ free (ready_ticks);
+
+ FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
+ if (FENCE_EXECUTING_INSNS (f))
+ FENCE_EXECUTING_INSNS (f)->block_remove (0,
+ FENCE_EXECUTING_INSNS (f)->length ());
+ if (FENCE_READY_TICKS (f))
+ memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
+ }
+ else
+ if (edge_new->probability > edge_old->probability)
+ {
+ delete_deps_context (FENCE_DC (f));
+ FENCE_DC (f) = dc;
+ vec_free (FENCE_EXECUTING_INSNS (f));
+ FENCE_EXECUTING_INSNS (f) = executing_insns;
+ free (FENCE_READY_TICKS (f));
+ FENCE_READY_TICKS (f) = ready_ticks;
+ FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
+ FENCE_CYCLE (f) = cycle;
+ }
+ else
+ {
+ /* Leave DC and CYCLE untouched. */
+ delete_deps_context (dc);
+ vec_free (executing_insns);
+ free (ready_ticks);
+ }
}
/* Fill remaining invariant fields. */
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
index 2314ad42402..245019b6587 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
@@ -57,19 +57,15 @@ fail:
}
#define FOR_EACH(VAR, START, STOP) \
- for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
+ for ((VAR) = (START); (VAR) < (STOP); (VAR++))
void fn_14 (void)
{
int i;
- FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10) /* { dg-message "3: this 'for' clause does not guard..." } */
foo (i);
bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'" } */
-/* { dg-begin-multiline-output "" }
- for ((VAR) = (START); (VAR) < (STOP); (VAR++))
- ^~~
- { dg-end-multiline-output "" } */
/* { dg-begin-multiline-output "" }
FOR_EACH (i, 0, 10)
^~~~~~~~
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
new file mode 100644
index 00000000000..f694c3935f2
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
@@ -0,0 +1,52 @@
+/* PR c/80076 */
+/* { dg-options "-Wmisleading-indentation" } */
+
+void foo(void);
+
+void test01(int flag) {
+#define bar() foo()
+ if (flag) /* { dg-warning "does not guard" } */
+ foo();
+ bar(); /* { dg-message "this statement" } */
+#undef bar
+}
+
+void test02(int flag) {
+#define bar() foo()
+ if (flag) /* { dg-warning "does not guard" } */
+ bar();
+ foo(); /* { dg-message "this statement" } */
+#undef bar
+}
+
+void test03(int flag) {
+#define bar() foo()
+ if (flag) /* { dg-warning "does not guard" } */
+ bar();
+ bar(); /* { dg-message "this statement" } */
+#undef bar
+}
+
+void test04(int flag, int num) {
+#define bar() \
+ { \
+ if (flag) /* { dg-warning "does not guard" } */ \
+ num = 0; \
+ num = 1; /* { dg-message "this statement" } */ \
+ }
+ bar();
+#undef bar
+}
+
+void test05(int flag, int num) {
+#define baz() (num = 1)
+#define bar() \
+ { \
+ if (flag) /* { dg-warning "does not guard" } */ \
+ num = 0; \
+ baz(); /* { dg-message "this statement" } */ \
+ }
+#define wrapper bar
+ wrapper();
+#undef bar
+}
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index 202c6bc7fdf..4265dcb4dfa 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -140,22 +140,22 @@ void fn_13 (void)
}
#define FOR_EACH(VAR, START, STOP) \
- for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
+ for ((VAR) = (START); (VAR) < (STOP); (VAR++))
void fn_14 (void)
{
int i;
- FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10) /* { dg-message "3: this 'for' clause does not guard..." } */
foo (i);
bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'" } */
}
#undef FOR_EACH
-#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "36: this 'for' clause does not guard..." } */
+#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++))
void fn_15 (void)
{
int i;
- FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10) /* { dg-message "3: this 'for' clause does not guard..." } */
foo (i);
bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'" } */
}
@@ -701,7 +701,7 @@ fn_37 (void)
int i;
#define EMPTY
-#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++) /* { dg-warning "this 'for' clause" } */
+#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++)
while (flagA); /* { dg-warning "3: this 'while' clause" } */
foo (0); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'while'" } */
@@ -759,15 +759,15 @@ fn_37 (void)
for (i = 0; i < 10; i++); /* { dg-warning "3: this 'for' clause" } */
foo (2); /* { dg-message "5: ...this statement" } */
- FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10); /* { dg-message "3: this 'for' clause does not guard..." } */
foo (2); /* { dg-message "5: ...this statement" } */
- FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10); /* { dg-message "3: this 'for' clause does not guard..." } */
{ /* { dg-message "5: ...this statement" } */
foo (3);
}
- FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10); /* { dg-message "3: this 'for' clause does not guard..." } */
{ /* { dg-message "3: ...this statement" } */
foo (3);
}
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 217f916fc35..44008be5c08 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1225,6 +1225,12 @@ LINEMAP_SYSP (const line_map_ordinary *ord_map)
return ord_map->sysp;
}
+const struct line_map *first_map_in_common (line_maps *set,
+ location_t loc0,
+ location_t loc1,
+ location_t *res_loc0,
+ location_t *res_loc1);
+
/* Return a positive value if PRE denotes the location of a token that
comes before the token of POST, 0 if PRE denotes the location of
the same token as the token for POST, and a negative value
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index a8d52861dee..5a74174579f 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -1289,7 +1289,7 @@ first_map_in_common_1 (line_maps *set,
virtual location of the token inside the resulting macro, upon
return of a non-NULL result. */
-static const struct line_map*
+const struct line_map*
first_map_in_common (line_maps *set,
location_t loc0,
location_t loc1,
--
2.28.0.497.g54e85e7af1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076]
2020-09-10 19:53 ` Patrick Palka
@ 2020-09-17 18:30 ` Patrick Palka
0 siblings, 0 replies; 8+ messages in thread
From: Patrick Palka @ 2020-09-17 18:30 UTC (permalink / raw)
To: Patrick Palka; +Cc: David Malcolm, gcc-patches
On Thu, 10 Sep 2020, Patrick Palka wrote:
> On Thu, 10 Sep 2020, David Malcolm wrote:
>
> > On Tue, 2020-07-28 at 20:22 -0400, Patrick Palka wrote:
> > > On Tue, 28 Jul 2020, David Malcolm wrote:
> > >
> > > > On Tue, 2020-07-28 at 15:50 -0400, Patrick Palka wrote:
> > > > > Currently the -Wmisleading-indentation warning doesn't do any
> > > > > analysis
> > > > > when the guarded statement or the statement after it is produced
> > > > > by a
> > > > > macro, as the mentioned PR illustrates. This means that we warn
> > > > > for:
> > > > >
> > > > > if (flag)
> > > > > foo ();
> > > > > bar ();
> > > > >
> > > > > and yet we don't warn for:
> > > > >
> > > > > #define BAR bar
> > > > > if (flag)
> > > > > foo ();
> > > > > BAR ();
> > > > >
> > > > > which seems like a surprising limitation.
> > > >
> > > > IIRC we were trying to keep things simple in the initial
> > > > implementation.
> > > >
> > > > > This patch extends the -Wmisleading-indentation implementation to
> > > > > support analyzing such statements and their tokens. This is done
> > > > > in
> > > > > the
> > > > > "natural" way by resolving the location of each of the three
> > > > > tokens
> > > > > to
> > > > > the token's macro expansion point. (Additionally, if the tokens
> > > > > all
> > > > > resolve to the same macro expansion point then we instead use
> > > > > their
> > > > > locations within the macro definition.) When these resolved
> > > > > locations
> > > > > are all different, then we can proceed with applying the warning
> > > > > heuristics to them as if no macros were involved.
> > > >
> > > > Thanks for working on this. I've only looked at the patch briefly,
> > > > but
> > > > so far it looks reasonable.
> > > >
> > > > Can you post some examples of what the output looks like for some
> > > > of
> > > > these cases? The diagnostics code has some logic for printing how
> > > > macros are unwound, so I'm wondering what we actually print for
> > > > these
> > > > cases, and if it looks intelligble to an end-user.
> > >
> > > For the code fragment mentioned above:
> > >
> > > int
> > > main()
> > > {
> > > #define BAR
> > > if (flag)
> > > foo ();
> > > BAR ();
> > > }
> > >
> > > we emit:
> > >
> > > test.cc: In function ‘int main()’:
> > > test.cc:7:3: warning: this ‘if’ clause does not guard... [-
> > > Wmisleading-indentation]
> > > 9 | if (flag)
> > > | ^~
> > > test.cc:8:15: note: ...this statement, but the latter is misleadingly
> > > indented as if it were guarded by the ‘if’
> > > 8 | #define BAR bar
> > > | ^~~
> > > test.cc:10:5: note: in expansion of macro ‘BAR’
> > > 11 | BAR ();
> > > | ^~~
> > >
> > >
> > > And when all tokens are generated from the same macro, e.g. for:
> > >
> > > int
> > > main()
> > > {
> > > #define BAR bar
> > > #define DO_STUFF \
> > > if (flag) \
> > > foo (); \
> > > BAR ();
> > > DO_STUFF;
> > > }
> > >
> > > we emit:
> > >
> > > test.cc: In function ‘int main()’:
> > > test.cc:11:5: warning: this ‘if’ clause does not guard... [-
> > > Wmisleading-indentation]
> > > 11 | if (flag) \
> > > | ^~
> > > test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
> > > 14 | DO_STUFF;
> > > | ^~~~~~~~
> > > test.cc:9:15: note: ...this statement, but the latter is misleadingly
> > > indented as if it were guarded by the ‘if’
> > > 9 | #define BAR bar
> > > | ^~~
> > > test.cc:13:7: note: in expansion of macro ‘BAR’
> > > 13 | BAR ();
> > > | ^~~
> > > test.cc:14:3: note: in expansion of macro ‘DO_STUFF’
> > > 14 | DO_STUFF;
> > > | ^~~~~~~~
> > >
> > > The automatic macro unwinding logic saves the day here and yields
> > > mostly
> > > legible output "for free". What do you think?
> >
> > Sorry about the delay in responding.
> >
> > I agree that the output is "mostly legible".
> >
> > The patch is OK as-is, and looking at the fixes you made to our source
> > tree I think it's usefully catching problematic code.
> >
>
> Thanks for your review!
I committed the original patch just now, and instead split out the
potential refinement to macro diagnostics as a followup patch for
separate consideration:
-- >8 --
Subject: [PATCH] c-family: Refine -Wmisleading-indentation diagnostics for
macros
This changes the locations of the warning and note emitted by
warn_for_misleading_indentation when the guard or next token is
generated from a macro. The intent is to make the emitted diagnostics
emphasize the location of the misleading indentation by omitting macro
expansion notes. For example, for the following fragment:
#define WHILE while
#define FOO foo
#define BAR bar
WHILE (cond)
FOO ();
BAR ();
before this patch we emit:
5:17: warning: this ‘while’ clause does not guard... [-Wmisleading-indentation]
5 | #define WHILE while
| ^~~~~
8:3: note: in expansion of macro ‘WHILE’
8 | WHILE (true)
| ^~~~~
7:15: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘while’
7 | #define BAR bar
| ^~~
10:5: note: in expansion of macro ‘BAR’
10 | BAR ();
| ^~~
and after the patch we emit:
8:3: warning: this ‘while’ clause does not guard... [-Wmisleading-indentation]
8 | WHILE (true)
| ^~~~~
10:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘while’
10 | BAR ();
| ^~~
gcc/c-family/ChangeLog:
* c-indentation.c (should_warn_for_misleading_indentation): Take
each token_indent_info argument by reference. Update the
token_indent_info::location of each of the supplied arguments.
(warn_for_misleading_indentation): Take each token_indent_info
argument by value.
* c-indentation.h (warn_for_misleading_indentation): Adjust
declaration accordingly.
gcc/testsuite/ChangeLog:
* c-c++-common/Wmisleading-indentation.c: Don't expect a "in
expansion of macro ..." note when the guard token is inside a
macro. Expect the "this clause does not guard" warning at the
macro expansion point instead of at the macro definition point
when the guard token is inside a macro.
* c-c++-common/Wmisleading-indentation-3.c: Likewise.
* c-c++-common/Wmisleading-indentation-5.c: Likewise.
---
gcc/c-family/c-indentation.c | 20 +++++++++++++------
gcc/c-family/c-indentation.h | 6 +++---
.../c-c++-common/Wmisleading-indentation-3.c | 8 ++------
.../c-c++-common/Wmisleading-indentation-5.c | 20 ++++++++-----------
.../c-c++-common/Wmisleading-indentation.c | 16 +++++++--------
5 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index 8b88a8adc7c..fa864cf2050 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -209,9 +209,9 @@ detect_intervening_unindent (const char *file,
description of that function below. */
static bool
-should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
- const token_indent_info &body_tinfo,
- const token_indent_info &next_tinfo)
+should_warn_for_misleading_indentation (token_indent_info &guard_tinfo,
+ token_indent_info &body_tinfo,
+ token_indent_info &next_tinfo)
{
/* Don't attempt to compare indentation if #line or # 44 "file"-style
directives are present, suggesting generated code.
@@ -329,6 +329,14 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
if (guard_loc == body_loc || body_loc == next_stmt_loc)
return false;
+ /* Now update the tokens' locations on the caller side too, so that the
+ diagnostics emitted by warn_for_misleading_indentation emphasize the
+ location of the problematic whitespace by omitting macro expansion
+ notes. */
+ guard_tinfo.location = guard_loc;
+ body_tinfo.location = body_loc;
+ next_tinfo.location = next_stmt_loc;
+
expanded_location body_exploc = expand_location (body_loc);
expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
expanded_location guard_exploc = expand_location (guard_loc);
@@ -634,9 +642,9 @@ guard_tinfo_to_string (enum rid keyword)
GUARD_KIND identifies the kind of clause e.g. "if", "else" etc. */
void
-warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
- const token_indent_info &body_tinfo,
- const token_indent_info &next_tinfo)
+warn_for_misleading_indentation (token_indent_info guard_tinfo,
+ token_indent_info body_tinfo,
+ token_indent_info next_tinfo)
{
/* Early reject for the case where -Wmisleading-indentation is disabled,
to avoid doing work only to have the warning suppressed inside the
diff --git a/gcc/c-family/c-indentation.h b/gcc/c-family/c-indentation.h
index e183d3aafb7..67400f6d43b 100644
--- a/gcc/c-family/c-indentation.h
+++ b/gcc/c-family/c-indentation.h
@@ -45,9 +45,9 @@ get_token_indent_info (const T *token)
}
extern void
-warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
- const token_indent_info &body_tinfo,
- const token_indent_info &next_tinfo);
+warn_for_misleading_indentation (token_indent_info guard_tinfo,
+ token_indent_info body_tinfo,
+ token_indent_info next_tinfo);
extern const char *
guard_tinfo_to_string (enum rid keyword);
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
index 2314ad42402..245019b6587 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
@@ -57,19 +57,15 @@ fail:
}
#define FOR_EACH(VAR, START, STOP) \
- for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
+ for ((VAR) = (START); (VAR) < (STOP); (VAR++))
void fn_14 (void)
{
int i;
- FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10) /* { dg-message "3: this 'for' clause does not guard..." } */
foo (i);
bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'" } */
-/* { dg-begin-multiline-output "" }
- for ((VAR) = (START); (VAR) < (STOP); (VAR++))
- ^~~
- { dg-end-multiline-output "" } */
/* { dg-begin-multiline-output "" }
FOR_EACH (i, 0, 10)
^~~~~~~~
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
index 12b53569ba7..f694c3935f2 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
@@ -4,10 +4,10 @@
void foo(void);
void test01(int flag) {
-#define bar() foo() /* { dg-message "this statement" } */
+#define bar() foo()
if (flag) /* { dg-warning "does not guard" } */
foo();
- bar(); /* { dg-message "in expansion of macro" } */
+ bar(); /* { dg-message "this statement" } */
#undef bar
}
@@ -20,23 +20,21 @@ void test02(int flag) {
}
void test03(int flag) {
-#define bar() foo() /* { dg-message "this statement" } */
+#define bar() foo()
if (flag) /* { dg-warning "does not guard" } */
bar();
- bar(); /* { dg-message "in expansion of macro" } */
+ bar(); /* { dg-message "this statement" } */
#undef bar
}
void test04(int flag, int num) {
#define bar() \
{ \
- if (flag) \
+ if (flag) /* { dg-warning "does not guard" } */ \
num = 0; \
- num = 1; \
+ num = 1; /* { dg-message "this statement" } */ \
}
bar();
-/* { dg-warning "does not guard" "" { target *-*-* } .-5 } */
-/* { dg-message "this statement" "" { target *-*-* } .-4 } */
#undef bar
}
@@ -44,13 +42,11 @@ void test05(int flag, int num) {
#define baz() (num = 1)
#define bar() \
{ \
- if (flag) \
+ if (flag) /* { dg-warning "does not guard" } */ \
num = 0; \
- baz(); \
+ baz(); /* { dg-message "this statement" } */ \
}
#define wrapper bar
wrapper();
-/* { dg-warning "does not guard" "" { target *-*-* } .-6 } */
-/* { dg-message "this statement" "" { target *-*-* } .-10 } */
#undef bar
}
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index 202c6bc7fdf..4265dcb4dfa 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -140,22 +140,22 @@ void fn_13 (void)
}
#define FOR_EACH(VAR, START, STOP) \
- for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
+ for ((VAR) = (START); (VAR) < (STOP); (VAR++))
void fn_14 (void)
{
int i;
- FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10) /* { dg-message "3: this 'for' clause does not guard..." } */
foo (i);
bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'" } */
}
#undef FOR_EACH
-#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "36: this 'for' clause does not guard..." } */
+#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++))
void fn_15 (void)
{
int i;
- FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10) /* { dg-message "3: this 'for' clause does not guard..." } */
foo (i);
bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'" } */
}
@@ -701,7 +701,7 @@ fn_37 (void)
int i;
#define EMPTY
-#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++) /* { dg-warning "this 'for' clause" } */
+#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++)
while (flagA); /* { dg-warning "3: this 'while' clause" } */
foo (0); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'while'" } */
@@ -759,15 +759,15 @@ fn_37 (void)
for (i = 0; i < 10; i++); /* { dg-warning "3: this 'for' clause" } */
foo (2); /* { dg-message "5: ...this statement" } */
- FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10); /* { dg-message "3: this 'for' clause does not guard..." } */
foo (2); /* { dg-message "5: ...this statement" } */
- FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10); /* { dg-message "3: this 'for' clause does not guard..." } */
{ /* { dg-message "5: ...this statement" } */
foo (3);
}
- FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+ FOR_EACH (i, 0, 10); /* { dg-message "3: this 'for' clause does not guard..." } */
{ /* { dg-message "3: ...this statement" } */
foo (3);
}
--
2.28.0.497.g54e85e7af1
>
> > That said, I wonder if the output could be improved for the macro case.
> >
> > Within should_warn_for_misleading_indentation the patch can update
> > guard_loc, body_loc and next_stmt_loc before expand_location is called
> > on them, but then in warn_for_misleading_indentation it uses the
> > original guard_tinfo.location and next_tinfo.location for the locations
> > of the warning and note. I have a hunch that the readability of the
> > diagnostic would be improved for the macro case by updating those
> > locations whenever the inputs to expand_location are updated. It would
> > lose some detail from the point of view of the user grokking macro
> > expansions, but perhaps is better given the specific focus of this
> > warning on whitespace. Does doing so help for the various cases you've
> > tried?
>
> This makes a lot of sense to me and it seems to work well in the cases
> I've tried.
>
> As before, for the code fragment:
>
> int
> main()
> {
> #define BAR
> if (flag)
> foo ();
> BAR ();
> }
>
> we now emit:
>
> test.cc: In function ‘int main()’:
> test.cc:7:3: warning: this ‘if’ clause does not guard... [-
> Wmisleading-indentation]
> 9 | if (flag)
> | ^~
> test.cc:10:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
> 11 | BAR ();
> | ^~~
>
>
> When all tokens are generated from the same macro, e.g. for:
>
> int
> main()
> {
> #define BAR bar
> #define DO_STUFF \
> if (flag) \
> foo (); \
> BAR ();
> DO_STUFF;
> }
>
> we emit... the same diagnostic as above :)
>
> The below patch implements the approach you outlined; some existing
> -Wmisleading-indentation testcases had to be adjusted accordingly. Is
> something like this what you had in mind?
>
> -- >8 --
>
> gcc/c-family/ChangeLog:
>
> PR c/80076
> * c-indentation.c (should_warn_for_misleading_indentation): Take
> each token_indent_info argument by reference. Move declarations
> of local variables closer to their first use. Handle virtual
> token locations by resolving them to their respective macro
> expansion points. If all three tokens are produced from the
> same macro expansion, then instead use their loci within the
> macro definition. Update the token_indent_info::location of
> each of the supplied arguments.
> (warn_for_misleading_indentation): Take each token_indent_info
> argument by value.
> * c-indentation.h (warn_for_misleading_indentation): Adjust
> declaration accordingly.
>
> gcc/objc/ChangeLog:
>
> PR c/80076
> * objc-gnu-runtime-abi-01.c
> (gnu_runtime_abi_01_get_class_super_ref): Reduce indentation of
> misleadingly indented return statements.
> * objc-next-runtime-abi-01.c
> (next_runtime_abi_01_get_class_super_ref): Likewise.
>
> gcc/ChangeLog:
>
> PR c/80076
> * gensupport.c (alter_attrs_for_subst_insn) <case SET_ATTR>:
> Reduce indentation of misleadingly indented code fragment.
> * lra-constraints.c (multi_block_pseudo_p): Likewise.
> * sel-sched-ir.c (merge_fences): Likewise.
>
> libcpp/ChangeLog:
>
> PR c/80076
> * include/line-map.h (first_map_in_common): Declare.
> * line-map.c (first_map_in_common): Remove static.
>
> gcc/testsuite/ChangeLog:
>
> PR c/80076
> * c-c++-common/Wmisleading-indentation.c: Don't expect a "in
> expansion of macro ..." note when the guard token is inside a
> macro. Expect the "this clause does not guard" warning at the
> macro expansion point instead of at the macro definition point
> when the guard token is inside a macro.
> * c-c++-common/Wmisleading-indentation-3.c: Likewise.
> * c-c++-common/Wmisleading-indentation-5.c: New test.
> ---
> gcc/c-family/c-indentation.c | 80 ++++++++++---
> gcc/c-family/c-indentation.h | 6 +-
> gcc/gensupport.c | 2 +-
> gcc/lra-constraints.c | 12 +-
> gcc/objc/objc-gnu-runtime-abi-01.c | 4 +-
> gcc/objc/objc-next-runtime-abi-01.c | 4 +-
> gcc/sel-sched-ir.c | 112 +++++++++---------
> .../c-c++-common/Wmisleading-indentation-3.c | 8 +-
> .../c-c++-common/Wmisleading-indentation-5.c | 52 ++++++++
> .../c-c++-common/Wmisleading-indentation.c | 16 +--
> libcpp/include/line-map.h | 6 +
> libcpp/line-map.c | 2 +-
> 12 files changed, 200 insertions(+), 104 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
>
> diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
> index d814f6f29e6..de7b426f35c 100644
> --- a/gcc/c-family/c-indentation.c
> +++ b/gcc/c-family/c-indentation.c
> @@ -209,23 +209,10 @@ detect_intervening_unindent (const char *file,
> description of that function below. */
>
> static bool
> -should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> - const token_indent_info &body_tinfo,
> - const token_indent_info &next_tinfo)
> +should_warn_for_misleading_indentation (token_indent_info &guard_tinfo,
> + token_indent_info &body_tinfo,
> + token_indent_info &next_tinfo)
> {
> - location_t guard_loc = guard_tinfo.location;
> - location_t body_loc = body_tinfo.location;
> - location_t next_stmt_loc = next_tinfo.location;
> -
> - enum cpp_ttype body_type = body_tinfo.type;
> - enum cpp_ttype next_tok_type = next_tinfo.type;
> -
> - /* Don't attempt to compare the indentation of BODY_LOC and NEXT_STMT_LOC
> - if either are within macros. */
> - if (linemap_location_from_macro_expansion_p (line_table, body_loc)
> - || linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
> - return false;
> -
> /* Don't attempt to compare indentation if #line or # 44 "file"-style
> directives are present, suggesting generated code.
>
> @@ -266,6 +253,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> } <- NEXT
> baz ();
> */
> + enum cpp_ttype next_tok_type = next_tinfo.type;
> if (next_tok_type == CPP_CLOSE_BRACE
> || next_tinfo.keyword == RID_ELSE)
> return false;
> @@ -287,6 +275,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> bar (); <- BODY
> baz (); <- NEXT
> */
> + enum cpp_ttype body_type = body_tinfo.type;
> if (body_type == CPP_OPEN_BRACE)
> return false;
>
> @@ -294,6 +283,59 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> if (next_tok_type == CPP_SEMICOLON)
> return false;
>
> + location_t guard_loc = guard_tinfo.location;
> + location_t body_loc = body_tinfo.location;
> + location_t next_stmt_loc = next_tinfo.location;
> +
> + /* Resolve each token location to the respective macro expansion
> + point that produced the token. */
> + if (linemap_location_from_macro_expansion_p (line_table, guard_loc))
> + guard_loc = linemap_resolve_location (line_table, guard_loc,
> + LRK_MACRO_EXPANSION_POINT, NULL);
> + if (linemap_location_from_macro_expansion_p (line_table, body_loc))
> + body_loc = linemap_resolve_location (line_table, body_loc,
> + LRK_MACRO_EXPANSION_POINT, NULL);
> + if (linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
> + next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
> + LRK_MACRO_EXPANSION_POINT, NULL);
> +
> + /* When all three tokens are produced from a single macro expansion, we
> + instead consider their loci inside that macro's definition. */
> + if (guard_loc == body_loc && body_loc == next_stmt_loc)
> + {
> + const line_map *guard_body_common_map
> + = first_map_in_common (line_table,
> + guard_tinfo.location, body_tinfo.location,
> + &guard_loc, &body_loc);
> + const line_map *body_next_common_map
> + = first_map_in_common (line_table,
> + body_tinfo.location, next_tinfo.location,
> + &body_loc, &next_stmt_loc);
> +
> + /* Punt on complicated nesting of macros. */
> + if (guard_body_common_map != body_next_common_map)
> + return false;
> +
> + guard_loc = linemap_resolve_location (line_table, guard_loc,
> + LRK_MACRO_DEFINITION_LOCATION, NULL);
> + body_loc = linemap_resolve_location (line_table, body_loc,
> + LRK_MACRO_DEFINITION_LOCATION, NULL);
> + next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
> + LRK_MACRO_DEFINITION_LOCATION,
> + NULL);
> + }
> +
> + /* Give up if the loci are not all distinct. */
> + if (guard_loc == body_loc || body_loc == next_stmt_loc)
> + return false;
> +
> + /* Now update the tokens' locations on the caller side too, so that the
> + diagnostics emitted by warn_for_misleading_indentation focus on the
> + problematic whitespace by omitting macro expansion notes. */
> + guard_tinfo.location = guard_loc;
> + body_tinfo.location = body_loc;
> + next_tinfo.location = next_stmt_loc;
> +
> expanded_location body_exploc = expand_location (body_loc);
> expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
> expanded_location guard_exploc = expand_location (guard_loc);
> @@ -599,9 +641,9 @@ guard_tinfo_to_string (enum rid keyword)
> GUARD_KIND identifies the kind of clause e.g. "if", "else" etc. */
>
> void
> -warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> - const token_indent_info &body_tinfo,
> - const token_indent_info &next_tinfo)
> +warn_for_misleading_indentation (token_indent_info guard_tinfo,
> + token_indent_info body_tinfo,
> + token_indent_info next_tinfo)
> {
> /* Early reject for the case where -Wmisleading-indentation is disabled,
> to avoid doing work only to have the warning suppressed inside the
> diff --git a/gcc/c-family/c-indentation.h b/gcc/c-family/c-indentation.h
> index e183d3aafb7..67400f6d43b 100644
> --- a/gcc/c-family/c-indentation.h
> +++ b/gcc/c-family/c-indentation.h
> @@ -45,9 +45,9 @@ get_token_indent_info (const T *token)
> }
>
> extern void
> -warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
> - const token_indent_info &body_tinfo,
> - const token_indent_info &next_tinfo);
> +warn_for_misleading_indentation (token_indent_info guard_tinfo,
> + token_indent_info body_tinfo,
> + token_indent_info next_tinfo);
> extern const char *
> guard_tinfo_to_string (enum rid keyword);
>
> diff --git a/gcc/gensupport.c b/gcc/gensupport.c
> index f2ad54f0c55..61691aadff1 100644
> --- a/gcc/gensupport.c
> +++ b/gcc/gensupport.c
> @@ -1501,7 +1501,7 @@ alter_attrs_for_subst_insn (class queue_elem * elem, int n_dup)
> case SET_ATTR:
> if (strchr (XSTR (sub, 1), ',') != NULL)
> XSTR (sub, 1) = duplicate_alternatives (XSTR (sub, 1), n_dup);
> - break;
> + break;
>
> case SET_ATTR_ALTERNATIVE:
> case SET:
> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
> index 161b721efb1..301c912cb21 100644
> --- a/gcc/lra-constraints.c
> +++ b/gcc/lra-constraints.c
> @@ -4776,12 +4776,12 @@ multi_block_pseudo_p (int regno)
> if (regno < FIRST_PSEUDO_REGISTER)
> return false;
>
> - EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
> - if (bb == NULL)
> - bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
> - else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
> - return true;
> - return false;
> + EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
> + if (bb == NULL)
> + bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
> + else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
> + return true;
> + return false;
> }
>
> /* Return true if LIST contains a deleted insn. */
> diff --git a/gcc/objc/objc-gnu-runtime-abi-01.c b/gcc/objc/objc-gnu-runtime-abi-01.c
> index d5862435c29..c9959a7e1e8 100644
> --- a/gcc/objc/objc-gnu-runtime-abi-01.c
> +++ b/gcc/objc/objc-gnu-runtime-abi-01.c
> @@ -821,7 +821,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> ucls_super_ref =
> objc_build_component_ref (imp->class_decl,
> get_identifier ("super_class"));
> - return ucls_super_ref;
> + return ucls_super_ref;
> }
> else
> {
> @@ -829,7 +829,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> uucls_super_ref =
> objc_build_component_ref (imp->meta_decl,
> get_identifier ("super_class"));
> - return uucls_super_ref;
> + return uucls_super_ref;
> }
> }
>
> diff --git a/gcc/objc/objc-next-runtime-abi-01.c b/gcc/objc/objc-next-runtime-abi-01.c
> index 5c34fcb05cb..233d89e75b5 100644
> --- a/gcc/objc/objc-next-runtime-abi-01.c
> +++ b/gcc/objc/objc-next-runtime-abi-01.c
> @@ -938,7 +938,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> ucls_super_ref =
> objc_build_component_ref (imp->class_decl,
> get_identifier ("super_class"));
> - return ucls_super_ref;
> + return ucls_super_ref;
> }
> else
> {
> @@ -946,7 +946,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
> uucls_super_ref =
> objc_build_component_ref (imp->meta_decl,
> get_identifier ("super_class"));
> - return uucls_super_ref;
> + return uucls_super_ref;
> }
> }
>
> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> index f58628ae92d..c8e086e4950 100644
> --- a/gcc/sel-sched-ir.c
> +++ b/gcc/sel-sched-ir.c
> @@ -722,63 +722,63 @@ merge_fences (fence_t f, insn_t insn,
> != BLOCK_FOR_INSN (last_scheduled_insn));
> }
>
> - /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
> - FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
> - SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> - {
> - if (succ == insn)
> - {
> - /* No same successor allowed from several edges. */
> - gcc_assert (!edge_old);
> - edge_old = si.e1;
> - }
> - }
> - /* Find edge of second predecessor (last_scheduled_insn->insn). */
> - FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
> - SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> - {
> - if (succ == insn)
> - {
> - /* No same successor allowed from several edges. */
> - gcc_assert (!edge_new);
> - edge_new = si.e1;
> - }
> - }
> + /* Find edge of first predecessor (last_scheduled_insn_old->insn). */
> + FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
> + SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> + {
> + if (succ == insn)
> + {
> + /* No same successor allowed from several edges. */
> + gcc_assert (!edge_old);
> + edge_old = si.e1;
> + }
> + }
> + /* Find edge of second predecessor (last_scheduled_insn->insn). */
> + FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
> + SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
> + {
> + if (succ == insn)
> + {
> + /* No same successor allowed from several edges. */
> + gcc_assert (!edge_new);
> + edge_new = si.e1;
> + }
> + }
>
> - /* Check if we can choose most probable predecessor. */
> - if (edge_old == NULL || edge_new == NULL)
> - {
> - reset_deps_context (FENCE_DC (f));
> - delete_deps_context (dc);
> - vec_free (executing_insns);
> - free (ready_ticks);
> -
> - FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
> - if (FENCE_EXECUTING_INSNS (f))
> - FENCE_EXECUTING_INSNS (f)->block_remove (0,
> - FENCE_EXECUTING_INSNS (f)->length ());
> - if (FENCE_READY_TICKS (f))
> - memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
> - }
> - else
> - if (edge_new->probability > edge_old->probability)
> - {
> - delete_deps_context (FENCE_DC (f));
> - FENCE_DC (f) = dc;
> - vec_free (FENCE_EXECUTING_INSNS (f));
> - FENCE_EXECUTING_INSNS (f) = executing_insns;
> - free (FENCE_READY_TICKS (f));
> - FENCE_READY_TICKS (f) = ready_ticks;
> - FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
> - FENCE_CYCLE (f) = cycle;
> - }
> - else
> - {
> - /* Leave DC and CYCLE untouched. */
> - delete_deps_context (dc);
> - vec_free (executing_insns);
> - free (ready_ticks);
> - }
> + /* Check if we can choose most probable predecessor. */
> + if (edge_old == NULL || edge_new == NULL)
> + {
> + reset_deps_context (FENCE_DC (f));
> + delete_deps_context (dc);
> + vec_free (executing_insns);
> + free (ready_ticks);
> +
> + FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
> + if (FENCE_EXECUTING_INSNS (f))
> + FENCE_EXECUTING_INSNS (f)->block_remove (0,
> + FENCE_EXECUTING_INSNS (f)->length ());
> + if (FENCE_READY_TICKS (f))
> + memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
> + }
> + else
> + if (edge_new->probability > edge_old->probability)
> + {
> + delete_deps_context (FENCE_DC (f));
> + FENCE_DC (f) = dc;
> + vec_free (FENCE_EXECUTING_INSNS (f));
> + FENCE_EXECUTING_INSNS (f) = executing_insns;
> + free (FENCE_READY_TICKS (f));
> + FENCE_READY_TICKS (f) = ready_ticks;
> + FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
> + FENCE_CYCLE (f) = cycle;
> + }
> + else
> + {
> + /* Leave DC and CYCLE untouched. */
> + delete_deps_context (dc);
> + vec_free (executing_insns);
> + free (ready_ticks);
> + }
> }
>
> /* Fill remaining invariant fields. */
> diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
> index 2314ad42402..245019b6587 100644
> --- a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
> +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
> @@ -57,19 +57,15 @@ fail:
> }
>
> #define FOR_EACH(VAR, START, STOP) \
> - for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
> + for ((VAR) = (START); (VAR) < (STOP); (VAR++))
>
> void fn_14 (void)
> {
> int i;
> - FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
> + FOR_EACH (i, 0, 10) /* { dg-message "3: this 'for' clause does not guard..." } */
> foo (i);
> bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'" } */
>
> -/* { dg-begin-multiline-output "" }
> - for ((VAR) = (START); (VAR) < (STOP); (VAR++))
> - ^~~
> - { dg-end-multiline-output "" } */
> /* { dg-begin-multiline-output "" }
> FOR_EACH (i, 0, 10)
> ^~~~~~~~
> diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
> new file mode 100644
> index 00000000000..f694c3935f2
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
> @@ -0,0 +1,52 @@
> +/* PR c/80076 */
> +/* { dg-options "-Wmisleading-indentation" } */
> +
> +void foo(void);
> +
> +void test01(int flag) {
> +#define bar() foo()
> + if (flag) /* { dg-warning "does not guard" } */
> + foo();
> + bar(); /* { dg-message "this statement" } */
> +#undef bar
> +}
> +
> +void test02(int flag) {
> +#define bar() foo()
> + if (flag) /* { dg-warning "does not guard" } */
> + bar();
> + foo(); /* { dg-message "this statement" } */
> +#undef bar
> +}
> +
> +void test03(int flag) {
> +#define bar() foo()
> + if (flag) /* { dg-warning "does not guard" } */
> + bar();
> + bar(); /* { dg-message "this statement" } */
> +#undef bar
> +}
> +
> +void test04(int flag, int num) {
> +#define bar() \
> + { \
> + if (flag) /* { dg-warning "does not guard" } */ \
> + num = 0; \
> + num = 1; /* { dg-message "this statement" } */ \
> + }
> + bar();
> +#undef bar
> +}
> +
> +void test05(int flag, int num) {
> +#define baz() (num = 1)
> +#define bar() \
> + { \
> + if (flag) /* { dg-warning "does not guard" } */ \
> + num = 0; \
> + baz(); /* { dg-message "this statement" } */ \
> + }
> +#define wrapper bar
> + wrapper();
> +#undef bar
> +}
> diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
> index 202c6bc7fdf..4265dcb4dfa 100644
> --- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
> +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
> @@ -140,22 +140,22 @@ void fn_13 (void)
> }
>
> #define FOR_EACH(VAR, START, STOP) \
> - for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
> + for ((VAR) = (START); (VAR) < (STOP); (VAR++))
>
> void fn_14 (void)
> {
> int i;
> - FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
> + FOR_EACH (i, 0, 10) /* { dg-message "3: this 'for' clause does not guard..." } */
> foo (i);
> bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'" } */
> }
> #undef FOR_EACH
>
> -#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "36: this 'for' clause does not guard..." } */
> +#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++))
> void fn_15 (void)
> {
> int i;
> - FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
> + FOR_EACH (i, 0, 10) /* { dg-message "3: this 'for' clause does not guard..." } */
> foo (i);
> bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'" } */
> }
> @@ -701,7 +701,7 @@ fn_37 (void)
> int i;
>
> #define EMPTY
> -#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++) /* { dg-warning "this 'for' clause" } */
> +#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++)
>
> while (flagA); /* { dg-warning "3: this 'while' clause" } */
> foo (0); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'while'" } */
> @@ -759,15 +759,15 @@ fn_37 (void)
> for (i = 0; i < 10; i++); /* { dg-warning "3: this 'for' clause" } */
> foo (2); /* { dg-message "5: ...this statement" } */
>
> - FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
> + FOR_EACH (i, 0, 10); /* { dg-message "3: this 'for' clause does not guard..." } */
> foo (2); /* { dg-message "5: ...this statement" } */
>
> - FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
> + FOR_EACH (i, 0, 10); /* { dg-message "3: this 'for' clause does not guard..." } */
> { /* { dg-message "5: ...this statement" } */
> foo (3);
> }
>
> - FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
> + FOR_EACH (i, 0, 10); /* { dg-message "3: this 'for' clause does not guard..." } */
> { /* { dg-message "3: ...this statement" } */
> foo (3);
> }
> diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> index 217f916fc35..44008be5c08 100644
> --- a/libcpp/include/line-map.h
> +++ b/libcpp/include/line-map.h
> @@ -1225,6 +1225,12 @@ LINEMAP_SYSP (const line_map_ordinary *ord_map)
> return ord_map->sysp;
> }
>
> +const struct line_map *first_map_in_common (line_maps *set,
> + location_t loc0,
> + location_t loc1,
> + location_t *res_loc0,
> + location_t *res_loc1);
> +
> /* Return a positive value if PRE denotes the location of a token that
> comes before the token of POST, 0 if PRE denotes the location of
> the same token as the token for POST, and a negative value
> diff --git a/libcpp/line-map.c b/libcpp/line-map.c
> index a8d52861dee..5a74174579f 100644
> --- a/libcpp/line-map.c
> +++ b/libcpp/line-map.c
> @@ -1289,7 +1289,7 @@ first_map_in_common_1 (line_maps *set,
> virtual location of the token inside the resulting macro, upon
> return of a non-NULL result. */
>
> -static const struct line_map*
> +const struct line_map*
> first_map_in_common (line_maps *set,
> location_t loc0,
> location_t loc1,
> --
> 2.28.0.497.g54e85e7af1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-17 18:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 19:50 [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076] Patrick Palka
2020-07-28 20:52 ` David Malcolm
2020-07-29 0:22 ` Patrick Palka
2020-09-10 17:01 ` David Malcolm
2020-09-10 19:53 ` Patrick Palka
2020-09-17 18:30 ` Patrick Palka
2020-08-10 13:46 ` Patrick Palka
2020-09-10 13:40 ` Patrick Palka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).