From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oo1-xc2e.google.com (mail-oo1-xc2e.google.com [IPv6:2607:f8b0:4864:20::c2e]) by sourceware.org (Postfix) with ESMTPS id 1F3F73858010 for ; Tue, 14 Dec 2021 22:06:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1F3F73858010 Received: by mail-oo1-xc2e.google.com with SMTP id g11-20020a4a754b000000b002c679a02b18so5319728oof.3 for ; Tue, 14 Dec 2021 14:06:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=pvzZDatZeOS4TNw+jKrS/TLFZhDCHVlgSXpjCfLNXg0=; b=YTI4i7TDRAumksdoeDZrxlRiOINajafAIjmYsmIjDZcRAd70BhefFmaTtkcWv2vTFr LkEWSMIAy3BE/g5XxHyVotAgBKUiOcxDn7vsZnek6nuNCGDiKJz+U0txLOxboWLsVCUu KawgGHJ/Sl9Ek1nSZiHGJCtnXSbwXpYKjHCASd6X8LUSI7hUx4kS+/HyRF75+Z9AEVSp NuPKMdCHuaT6ftUj2xfBJARGc8lRKVQr6XdK7Sz99+uq4E9iQcBUuFCAhwjbXpcnZrBn 5jA9yOzmOAquzCYT/EsoFFQQwNbEilK1c6sfDIzT8RYvck/YEKVXgtD4Xe10gVDPBEYg npIw== X-Gm-Message-State: AOAM533bVkhA2Ca6a9tKnuPqQlaYNccb6Xr1HkKW5KsV8EC7Bo6L869b rP7ohAlFEgCHSZqB710MuuVlDsFouGs= X-Google-Smtp-Source: ABdhPJxZUN03eMqFdIZmBoB67cYbG5dkOxqgNvbJA+oU0eGAeCQAOI/WaubrhbZQ0uKdqz2iw/Lbgw== X-Received: by 2002:a4a:2101:: with SMTP id u1mr5219026oou.39.1639519574309; Tue, 14 Dec 2021 14:06:14 -0800 (PST) Received: from [192.168.0.41] (184-96-227-137.hlrn.qwest.net. [184.96.227.137]) by smtp.gmail.com with ESMTPSA id l23sm12806oti.16.2021.12.14.14.06.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Dec 2021 14:06:13 -0800 (PST) Subject: Re: [Patch]Enable -Wuninitialized + -ftrivial-auto-var-init for address taken variables To: Qing Zhao Cc: gcc Patches References: <53205959-8D66-47D8-B88C-8B1D65B5D008@oracle.com> <7BD4CE27-1053-4FD5-AA2A-40989AABC520@oracle.com> <7FA47305-BB75-4080-97FD-CF57A5EF99BA@oracle.com> From: Martin Sebor Message-ID: <7a0183c2-aa22-98a5-eab4-9576837ee37a@gmail.com> Date: Tue, 14 Dec 2021 15:06:12 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <7FA47305-BB75-4080-97FD-CF57A5EF99BA@oracle.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Dec 2021 22:06:16 -0000 On 12/14/21 9:43 AM, Qing Zhao wrote: > Hi, > >> On Dec 9, 2021, at 12:13 PM, Qing Zhao via Gcc-patches wrote: >>> >>>> + return; >>>> + >>>> + /* Get the variable declaration location from the def_stmt. */ >>>> + var_decl_loc = gimple_location (def_stmt); >>>> + >>>> + /* The LHS of the call is a temporary variable, we use it as a >>>> + placeholder to record the information on whether the warning >>>> + has been issued or not. */ >>>> + repl_var = gimple_call_lhs (def_stmt); >>>> + } >>>> } >>>> - if (var == NULL_TREE) >>>> + if (var == NULL_TREE && var_name == NULL_TREE) >>>> return; >>>> /* Avoid warning if we've already done so or if the warning has been >>>> @@ -207,36 +245,56 @@ warn_uninit (opt_code opt, tree t, tree var, const char *gmsgid, >>>> if (((warning_suppressed_p (context, OPT_Wuninitialized) >>>> || (gimple_assign_single_p (context) >>>> && get_no_uninit_warning (gimple_assign_rhs1 (context))))) >>>> - || get_no_uninit_warning (var)) >>>> + || (var && get_no_uninit_warning (var)) >>>> + || (repl_var && get_no_uninit_warning (repl_var))) >>>> return; >>>> /* Use either the location of the read statement or that of the PHI >>>> argument, or that of the uninitialized variable, in that order, >>>> whichever is valid. */ >>>> - location_t location; >>>> + location_t location = UNKNOWN_LOCATION; >>>> if (gimple_has_location (context)) >>>> location = gimple_location (context); >>>> else if (phi_arg_loc != UNKNOWN_LOCATION) >>>> location = phi_arg_loc; >>>> - else >>>> + else if (var) >>>> location = DECL_SOURCE_LOCATION (var); >>>> + else if (var_name) >>>> + location = var_decl_loc; >>>> + >>>> location = linemap_resolve_location (line_table, location, >>>> LRK_SPELLING_LOCATION, NULL); >>>> auto_diagnostic_group d; >>>> - if (!warning_at (location, opt, gmsgid, var)) >>>> + char *gmsgid_final = XNEWVEC (char, strlen (gmsgid) + 5); >>>> + gmsgid_final[0] = 0; >>>> + if (var) >>>> + strcat (gmsgid_final, "%qD "); >>>> + else if (var_name) >>>> + strcat (gmsgid_final, "%qs "); >>>> + strcat (gmsgid_final, gmsgid); >>>> + >>>> + if (var && !warning_at (location, opt, gmsgid_final, var)) >>>> + return; >>>> + else if (var_name && !warning_at (location, opt, gmsgid_final, var_name_str)) >>>> return; >>> >>> Dynamically creating the string seems quite cumbersome here, and >>> it leaks the allocated block. I wonder if it might be better to >>> remove the gmsgid argument from the function and assign it to >>> one of the literals based on the other arguments. >>> >>> Since only one of var and var_name is used, I also wonder if >>> the %qs form could be used for both to simplify the overall >>> logic. (I.e., get the IDENTIFIER_POINTER string from var and >>> use it instead of %qD). > > Looks like that using “%qs” + get the IDENTIFIER_POINTER string from var did not work very well for the following testing case: > > 1 /* PR tree-optimization/45083 */ > 2 /* { dg-do compile } */ > 3 /* { dg-options "-O2 -Wuninitialized" } */ > 4 > 5 struct S { char *a; unsigned b; unsigned c; }; > 6 extern int foo (const char *); > 7 extern void bar (int, int); > 8 > 9 static void > 10 baz (void) > 11 { > 12 struct S cs[1]; /* { dg-message "was declared here" } */ > 13 switch (cs->b) /* { dg-warning "cs\[^\n\r\]*\\.b\[^\n\r\]*is used uninitialized" } */ > 14 { > 15 case 101: > 16 if (foo (cs->a)) /* { dg-warning "cs\[^\n\r\]*\\.a\[^\n\r\]*may be used uninitialized" } */ > 17 bar (cs->c, cs->b); /* { dg-warning "cs\[^\n\r\]*\\.c\[^\n\r\]*may be used uninitialized" } */ > 18 } > 19 } > 20 > 21 void > 22 test (void) > 23 { > 24 baz (); > 25 } > > > For the uninitialized usages at line 13, 16, 17: the IDENTIFIER_POINTER string of var are: > cs$0$b, cs$0$a ,cs$0$c > > However, with %qD, they are printed as cs[0].b, cs[0].a, cs[0].c > But with %qs, they are printed as cs$0$b, cs$0$a ,cs$0$c. > > Looks like that %qD does not simplify print out the IDENTIFIER_POINTER string directly, it specially handle it for some cases. > > I tried to see how %qD specially handle the strings, but didn’t get it so far. > > Do you know where the %qD handle this case specially? In the front end's pretty printer where it handles %D (e.g., for C in c_tree_printer in c/c-objc-common.c). For VARs with DECL_HAS_DEBUG_EXPR_P (temp) the code uses DECL_DEBUG_EXPR(). There's also print_generic_expr_to_str(tree) that formats a decl or an expression to a dynamically allocated string (the string needs to be freed). Martin > > Thanks. > > Qing > > >> Both are good suggestions, I will try to update the code based on this. >> >> Thanks again. >> >> Qing >>> >> >