From: Qing Zhao <qing.zhao@oracle.com>
To: Kees Cook <keescook@chromium.org>
Cc: richard Sandiford <richard.sandiford@arm.com>,
Richard Biener <rguenther@suse.de>,
Gcc-patches Qing Zhao via <gcc-patches@gcc.gnu.org>
Subject: Re: [patch for gcc12 stage1][version 2] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
Date: Thu, 8 Apr 2021 10:22:21 -0500 [thread overview]
Message-ID: <2AD3221B-1EF5-4DCB-8348-C279FBB5E460@oracle.com> (raw)
In-Reply-To: <202104071509.A77994FC1F@keescook>
Hi, Kees,
Thanks a lot for your testing on the linux kernel, I am so happy that this time it works well.
> On Apr 7, 2021, at 5:19 PM, Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Mar 24, 2021 at 04:21:49PM -0500, Qing Zhao wrote:
>> This is the 2nd version of the patch for the new security feature for GCC.
>>
>> Could you please take a look at it and let me know any comments and issues.
>
> This behaves perfectly as far as I'm able to test in the Linux kernel!
> Thank you!
>
> For comparison to v1, here's the stack init test output for version 2 of the patch:
>
> test_stackinit: u8_zero ok
> test_stackinit: u16_zero ok
> test_stackinit: u32_zero ok
> test_stackinit: u64_zero ok
> test_stackinit: char_array_zero ok
> test_stackinit: small_hole_zero ok
> test_stackinit: big_hole_zero ok
> test_stackinit: trailing_hole_zero ok
> test_stackinit: packed_zero ok
> test_stackinit: small_hole_dynamic_partial ok
> test_stackinit: big_hole_dynamic_partial ok
> test_stackinit: trailing_hole_dynamic_partial ok
> test_stackinit: packed_dynamic_partial ok
> test_stackinit: small_hole_static_partial ok
> test_stackinit: big_hole_static_partial ok
> test_stackinit: trailing_hole_static_partial ok
> test_stackinit: packed_static_partial ok
> test_stackinit: small_hole_static_all ok
> test_stackinit: big_hole_static_all ok
> test_stackinit: trailing_hole_static_all ok
> test_stackinit: packed_static_all ok
> test_stackinit: small_hole_dynamic_all ok
> test_stackinit: big_hole_dynamic_all ok
> test_stackinit: trailing_hole_dynamic_all ok
> test_stackinit: packed_dynamic_all ok
> test_stackinit: small_hole_runtime_partial ok
> test_stackinit: big_hole_runtime_partial ok
> test_stackinit: trailing_hole_runtime_partial ok
> test_stackinit: packed_runtime_partial ok
> test_stackinit: small_hole_runtime_all ok
> test_stackinit: big_hole_runtime_all ok
> test_stackinit: trailing_hole_runtime_all ok
> test_stackinit: packed_runtime_all ok
> test_stackinit: u8_none ok
> test_stackinit: u16_none ok
> test_stackinit: u32_none ok
> test_stackinit: u64_none ok
> test_stackinit: char_array_none ok
> test_stackinit: switch_1_none XFAIL (uninit bytes: 8)
> test_stackinit: switch_2_none XFAIL (uninit bytes: 8)
> test_stackinit: small_hole_none ok
> test_stackinit: big_hole_none ok
> test_stackinit: trailing_hole_none ok
> test_stackinit: packed_none ok
> test_stackinit: user ok
> test_stackinit: all tests passed!
>
> The switch cases are an "expected fail" still, and that's totally fine
> for now[1]. Those were all purged from real kernel code anyway. ;)
>
> So that's a big "Ack" from me. :)
>
> What are next steps for this patch?
My understanding is, it needs to be reviewed and get approved by the global reviewers first,
Currently, I am waiting for any comments from global reviewers in order to be able to push
this patch into gcc12 as early as possible.
> I know a lot of people are looking
> forward to it. (And then long-open bug[2] for auto-init can be closed.)
>
> Thanks again!
Thank you!
Qing
>
> -Kees
>
> [1] Clang doesn't handle this either https://bugs.llvm.org/show_bug.cgi?id=44916 <https://bugs.llvm.org/show_bug.cgi?id=44916>
> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87210 <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87210>
>
>>
>> Thanks.
>>
>> Qing
>>
>> ******compared to Version 1, this version added the following new features to address Kees’s comments:
>>
>> 1. correctly handle VLA inside a structure for pattern initialization.
>> In tree.c (build_pattern_cst):
>>
>> + /* if the field is a variable length array, it should be the last
>> + field of the record, and no need to initialize. */
>> + if (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
>> + && TYPE_SIZE (TREE_TYPE (field)) == NULL_TREE
>> + && ((TYPE_DOMAIN (TREE_TYPE (field)) != NULL_TREE
>> + && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (field)))
>> + == NULL_TREE)
>> + || TYPE_DOMAIN (TREE_TYPE (field)) == NULL_TREE))
>> + continue;
>>
>> 2. initialize all paddings to zero when -ftrivial-auto-var-init is present.
>> In expr.c (store_constructor):
>>
>> Clear the whole structure when
>> -ftrivial-auto-var-init and the structure has paddings.
>>
>> In gimplify.c (gimplify_init_constructor):
>>
>> Clear the whole structure when
>> -ftrivial-auto-var-init and the structure has paddings.
>>
>> As agreed with Kees, treat the issue related to auto variables outside of the cases and inside the switch as a low priority one.
>>
>> 3. Add the following new testing cases for the above 1 and 2:
>>
>> * c-c++-common/auto-init-13.c: New test.
>> * c-c++-common/auto-init-14.c: New test.
>>
>> * gcc.target/aarch64/auto-init-9.c: New test.
>> * gcc.target/aarch64/auto-init-10.c: New test.
>> * gcc.target/aarch64/auto-init-11.c: New test.
>> * gcc.target/aarch64/auto-init-12.c: New test.
>> * gcc.target/aarch64/auto-init-13.c: New test.
>> * gcc.target/aarch64/auto-init-14.c: New test.
>> * gcc.target/aarch64/auto-init-15.c: New test.
>> * gcc.target/aarch64/auto-init-16.c: New test.
>> * gcc.target/aarch64/auto-init-17.c: New test.
>> * gcc.target/aarch64/auto-init-18.c: New test.
>> * gcc.target/aarch64/auto-init-19.c: New test.
>> * gcc.target/aarch64/auto-init-20.c: New test.
>>
>> * gcc.target/i386/auto-init-9.c: New test.
>> * gcc.target/i386/auto-init-10.c: New test.
>> * gcc.target/i386/auto-init-11.c: New test.
>> * gcc.target/i386/auto-init-12.c: New test.
>> * gcc.target/i386/auto-init-13.c: New test.
>> * gcc.target/i386/auto-init-14.c: New test.
>> * gcc.target/i386/auto-init-15.c: New test.
>> * gcc.target/i386/auto-init-16.c: New test.
>> * gcc.target/i386/auto-init-17.c: New test.
>> * gcc.target/i386/auto-init-18.c: New test.
>> * gcc.target/i386/auto-init-19.c: New test.
>> * gcc.target/i386/auto-init-20.c: New test.
>>
>> 4. Update the default approach as D, then when specify -ftrivial-auto-var-init, the default approach is the “.DEFERRED_INIT”
>> Approach. No need to add -fauto-var-init-approach=D anymore.
>>
>> If we need to compare approach A and D, we can add -fauto-var-init-approach=A to get that implementation.
>>
>> 5. Delete all -fauto-var-init-approach=D in the testing cases.
>>
>> ****** others are the same as Version 1, please see the version 1 description at:
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565581.html
>>
>>
>> *******Changelog:
>>
>> gcc/:
>>
>> 2021-03-24 qing zhao <qing.zhao@oracle.com>
>>
>> * common.opt (ftrivial-auto-var-init=): New.
>> (fauto-var-init-approach=): Likewise.
>> * doc/extend.texi: Document the uninitialized attribute.
>> * doc/invoke.texi: Document -ftrivial-auto-var-init.
>> * expr.c (store_constructor): Clear the whole structure when
>> -ftrivial-auto-var-init and the structure has paddings.
>> * flag-types.h (enum auto_init_type): New enumerated type
>> auto_init_type.
>> (enum auto_init_approach): New enumerated type auto_init_approach.
>> * gimple.h (enum gf_mask): Add GF_CALL_MEMSET_FOR_UNINIT case.
>> (gimple_call_set_memset_for_uninit): New function.
>> (gimple_call_memset_for_uninit_p): Likewise.
>> * gimplify.c (gimplify_vla_decl): Add initialization to vla per users'
>> requests.
>> (build_deferred_init): New function.
>> (gimple_add_init_for_auto_var): Likewise.
>> (gimplify_decl_expr): Add initialization to automatic variables per
>> users' requests.
>> (gimplify_init_constructor): Clear the whole structure when
>> -ftrivial-auto-var-init and the structure has paddings.
>> * internal-fn.c (expand_DEFERRED_INIT): New function.
>> * internal-fn.def (DEFERRED_INIT): New internal function.
>> * tree-cfg.c (verify_gimple_call): Skip calls to DEFERRED_INIT.
>> * tree-core.h (tree_decl_with_vis): Add uninitialized field.
>> * tree-sra.c (sra_stats): Add two new fields deferred_init and
>> subtree_deferred_init.
>> (generate_subtree_deferred_init): New function.
>> (sra_modify_deferred_init): Likewise.
>> (sra_modify_function_body): Handle calls to DEFERRED_INIT specially.
>> * tree-ssa-structalias.c (find_func_aliases_for_deferred_init): New
>> function.
>> (find_func_aliases_for_call): Handle calls to DEFERRED_INIT specially.
>> * tree-ssa-uninit.c (warn_uninit): Handle calls to DEFERRED_INIT
>> specially.
>> (check_defs): Handle calls to DEFERRED_INIT and MEMSET for uninitialized
>> variable specially.
>> (warn_uninitialized_vars): Handle calls to DEFERRED_INIT specially.
>> * tree-ssa.c (ssa_undefined_value_p): Handle calls to DEFERRED_INIT
>> specially.
>> * tree.c (build_pattern_cst): New function.
>> (type_has_padding): Likewise.
>> * tree.h (DECL_UNINITIALIZED): New macro.
>> (build_pattern_cst): New declaration.
>> (type_has_padding): Likewise.
>>
>> gcc/c-family/:
>>
>> 2021-03-24 qing zhao <qing.zhao@oracle.com>
>>
>> * c-attribs.c (handle_uninitialized_attribute): New function.
>> (c_common_attribute_table): Add "uninitialized" attribute.
>>
>> gcc/testsuite/:
>>
>> 2021-03-24 qing zhao <qing.zhao@oracle.com>
>>
>> * c-c++-common/auto-init-1.c: New test.
>> * c-c++-common/auto-init-10.c: New test.
>> * c-c++-common/auto-init-11.c: New test.
>> * c-c++-common/auto-init-12.c: New test.
>> * c-c++-common/auto-init-13.c: New test.
>> * c-c++-common/auto-init-14.c: New test.
>> * c-c++-common/auto-init-2.c: New test.
>> * c-c++-common/auto-init-3.c: New test.
>> * c-c++-common/auto-init-4.c: New test.
>> * c-c++-common/auto-init-5.c: New test.
>> * c-c++-common/auto-init-6.c: New test.
>> * c-c++-common/auto-init-7.c: New test.
>> * c-c++-common/auto-init-8.c: New test.
>> * c-c++-common/auto-init-9.c: New test.
>> * c-c++-common/auto-init-esra.c: New test.
>> * g++.dg/auto-init-uninit-pred-1_a.C: New test.
>> * g++.dg/auto-init-uninit-pred-1_b.C: New test.
>> * g++.dg/auto-init-uninit-pred-2_a.C: New test.
>> * g++.dg/auto-init-uninit-pred-2_b.C: New test.
>> * g++.dg/auto-init-uninit-pred-3_a.C: New test.
>> * g++.dg/auto-init-uninit-pred-3_b.C: New test.
>> * g++.dg/auto-init-uninit-pred-4.C: New test.
>> * g++.dg/auto-init-uninit-pred-loop-1_a.cc: New test.
>> * g++.dg/auto-init-uninit-pred-loop-1_b.cc: New test.
>> * g++.dg/auto-init-uninit-pred-loop-1_c.cc: New test.
>> * g++.dg/auto-init-uninit-pred-loop_1.cc: New test.
>> * gcc.dg/auto-init-uninit-1.c: New test.
>> * gcc.dg/auto-init-uninit-11.c: New test.
>> * gcc.dg/auto-init-uninit-12.c: New test.
>> * gcc.dg/auto-init-uninit-13.c: New test.
>> * gcc.dg/auto-init-uninit-14.c: New test.
>> * gcc.dg/auto-init-uninit-15.c: New test.
>> * gcc.dg/auto-init-uninit-16.c: New test.
>> * gcc.dg/auto-init-uninit-17.c: New test.
>> * gcc.dg/auto-init-uninit-18.c: New test.
>> * gcc.dg/auto-init-uninit-19.c: New test.
>> * gcc.dg/auto-init-uninit-2.c: New test.
>> * gcc.dg/auto-init-uninit-20.c: New test.
>> * gcc.dg/auto-init-uninit-21.c: New test.
>> * gcc.dg/auto-init-uninit-22.c: New test.
>> * gcc.dg/auto-init-uninit-23.c: New test.
>> * gcc.dg/auto-init-uninit-24.c: New test.
>> * gcc.dg/auto-init-uninit-25.c: New test.
>> * gcc.dg/auto-init-uninit-26.c: New test.
>> * gcc.dg/auto-init-uninit-3.c: New test.
>> * gcc.dg/auto-init-uninit-34.c: New test.
>> * gcc.dg/auto-init-uninit-36.c: New test.
>> * gcc.dg/auto-init-uninit-37.c: New test.
>> * gcc.dg/auto-init-uninit-4.c: New test.
>> * gcc.dg/auto-init-uninit-5.c: New test.
>> * gcc.dg/auto-init-uninit-6.c: New test.
>> * gcc.dg/auto-init-uninit-8.c: New test.
>> * gcc.dg/auto-init-uninit-9.c: New test.
>> * gcc.dg/auto-init-uninit-A.c: New test.
>> * gcc.dg/auto-init-uninit-B.c: New test.
>> * gcc.dg/auto-init-uninit-C.c: New test.
>> * gcc.dg/auto-init-uninit-H.c: New test.
>> * gcc.dg/auto-init-uninit-I.c: New test.
>> * gcc.target/aarch64/auto-init-1.c: New test.
>> * gcc.target/aarch64/auto-init-10.c: New test.
>> * gcc.target/aarch64/auto-init-11.c: New test.
>> * gcc.target/aarch64/auto-init-12.c: New test.
>> * gcc.target/aarch64/auto-init-13.c: New test.
>> * gcc.target/aarch64/auto-init-14.c: New test.
>> * gcc.target/aarch64/auto-init-15.c: New test.
>> * gcc.target/aarch64/auto-init-16.c: New test.
>> * gcc.target/aarch64/auto-init-17.c: New test.
>> * gcc.target/aarch64/auto-init-18.c: New test.
>> * gcc.target/aarch64/auto-init-19.c: New test.
>> * gcc.target/aarch64/auto-init-2.c: New test.
>> * gcc.target/aarch64/auto-init-20.c: New test.
>> * gcc.target/aarch64/auto-init-3.c: New test.
>> * gcc.target/aarch64/auto-init-4.c: New test.
>> * gcc.target/aarch64/auto-init-5.c: New test.
>> * gcc.target/aarch64/auto-init-6.c: New test.
>> * gcc.target/aarch64/auto-init-7.c: New test.
>> * gcc.target/aarch64/auto-init-8.c: New test.
>> * gcc.target/aarch64/auto-init-9.c: New test.
>> * gcc.target/i386/auto-init-1.c: New test.
>> * gcc.target/i386/auto-init-10.c: New test.
>> * gcc.target/i386/auto-init-11.c: New test.
>> * gcc.target/i386/auto-init-12.c: New test.
>> * gcc.target/i386/auto-init-13.c: New test.
>> * gcc.target/i386/auto-init-14.c: New test.
>> * gcc.target/i386/auto-init-15.c: New test.
>> * gcc.target/i386/auto-init-16.c: New test.
>> * gcc.target/i386/auto-init-17.c: New test.
>> * gcc.target/i386/auto-init-18.c: New test.
>> * gcc.target/i386/auto-init-19.c: New test.
>> * gcc.target/i386/auto-init-2.c: New test.
>> * gcc.target/i386/auto-init-20.c: New test.
>> * gcc.target/i386/auto-init-3.c: New test.
>> * gcc.target/i386/auto-init-4.c: New test.
>> * gcc.target/i386/auto-init-5.c: New test.
>> * gcc.target/i386/auto-init-6.c: New test.
>> * gcc.target/i386/auto-init-7.c: New test.
>> * gcc.target/i386/auto-init-8.c: New test.
>> * gcc.target/i386/auto-init-9.c: New test.
>>
>> ******The complete patch is attached as following:
>>
>
>
>>
>>
>
>
> --
> Kees Cook
next prev parent reply other threads:[~2021-04-08 15:22 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-24 21:21 Qing Zhao
2021-04-07 14:44 ` Qing Zhao
2021-04-07 22:19 ` Kees Cook
2021-04-08 15:22 ` Qing Zhao [this message]
2021-04-23 19:05 ` Richard Sandiford
2021-04-23 19:33 ` Kees Cook
2021-04-26 15:09 ` Qing Zhao
2021-04-26 17:47 ` Richard Sandiford
2021-04-26 20:02 ` Qing Zhao
2021-04-27 6:30 ` Richard Biener
2021-04-27 15:10 ` Qing Zhao
2021-05-05 17:29 ` Qing Zhao
2021-05-05 14:41 ` Qing Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2AD3221B-1EF5-4DCB-8348-C279FBB5E460@oracle.com \
--to=qing.zhao@oracle.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=keescook@chromium.org \
--cc=rguenther@suse.de \
--cc=richard.sandiford@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).