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
> >
> >
>
