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