On 7/28/21 3:23 AM, Richard Biener wrote:
On Fri, Jul 16, 2021 at 12:42 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
A number of access warnings as well as their supporting
infrastructure (compute_objsize et al.) are implemented in
builtins.{c,h} where they (mostly) operate on trees and run
just before RTL expansion.
This setup may have made sense initially when the warnings were
very simple and didn't perform any CFG analysis, but it's becoming
a liability. The code has grown both in size and in complexity,
might need to examine the CFG to improve detection, and in some
cases might achieve a better S/R ratio if run earlier. Running
the warning code on trees is also slower because it doesn't
benefit from the SSA_NAME caching provided by the pointer_query
class. Finally, having the code there is also an impediment to
maintainability as warnings and builtin expansion are unrelated
to each other and contributors to one area shouldn't need to wade
through unrelated code (similar for patch reviewers).
The attached change introduces a new warning pass and a couple of
new source and headers and, as the first step, moves the warning
code from builtins.{c,h} there. To keep the initial changes as
simple as possible the pass only runs a subset of existing
warnings: -Wfree-nonheap-object, -Wmismatched-dealloc, and
-Wmismatched-new-delete. The others (-Wstringop-overflow and
-Wstringop-overread) still run on the tree representation and
are still invoked from builtins.c or elsewhere.
The changes have no functional impact either on codegen or on
warnings. I tested them on x86_64-linux.
As the next step I plan to change the -Wstringop-overflow and
-Wstringop-overread code to run on the GIMPLE IL in the new pass
instead of on trees in builtins.c.
That's the maybe_warn_rdwr_sizes thing?
Among others, yes. It includes most buffer overflow and overread
warnings.
+ gimple *stmt = gsi_stmt (si);
+ if (!is_gimple_call (stmt))
+ continue;
+
+ check (as_a <gcall *>(stmt));
if (gcall *call = dyn_cast <gcall *> (gsi_stmt (si)))
check (call);
might be more C++-ish.
Sure, I can do that.
The patch looks OK - I skimmed it as mostly moving things
around plus adding a new pass.
Okay, thanks. I have retested the updated change and pushed it in
r12-2581. As a reminder, the git show output for builtins.c looks
considerably different from the patch I posted because of what I
mentioned below.
Martin
Thanks,
Richard.
Martin
PS The builtins.c diff produced by git diff was much bigger than
the changes justify. It seems that the code removal somehow
confused it. To make review easy I replaced it with a plain
unified diff of builtins.c that doesn't suffer from the problem.
