Bug 97424 - Warn on invalid shift amount after inlining
Summary: Warn on invalid shift amount after inlining
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 11.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: new-warning, new_warning
  Show dependency treegraph
 
Reported: 2020-10-14 16:02 UTC by Florian Weimer
Modified: 2020-12-26 21:38 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Weimer 2020-10-14 16:02:08 UTC
Consider this program:

#include <stdint.h>

static inline uint32_t
_dl_hwcaps_subdirs_build_bitmask (int subdirs, int active)
{
  /* Leading subdirectories that are not active.  */
  int inactive = subdirs - active;
  if (inactive == 32)
    return 0;

  uint32_t mask;
  if (subdirs != 32)
    mask = (1 << subdirs) - 1;
  else
    mask = -1;
  return mask ^ ((1U << inactive) - 1);
}

void f1 (int);

void
f2 (void)
{
  f1 (_dl_hwcaps_subdirs_build_bitmask (1, 2));
  f1 (_dl_hwcaps_subdirs_build_bitmask (33, 31));
}

This has invalid shifts involving a negative shift amount and larger-than-width shift amount. This does not result in a warning because the current shift warnings are implemented in the front end. But the computed values as the argument to f1 are garbage, so it would make sense to warn.
Comment 1 Jakub Jelinek 2020-10-14 16:45:41 UTC
Such a warning would suffer from the usual pain of late warnings, warning even about cases of this in unreachable code that the compiler can't prove is unreachable.
An alternative to this is -fsanitize=undefined which detects only the reachable cases at runtime.
Comment 2 Florian Weimer 2020-10-14 16:48:26 UTC
Indeed, Martin Sebor has suggested that it would have to be coupled with __builtin_warning: https://gcc.gnu.org/legacy-ml/gcc-patches/2019-10/msg01015.html
Comment 3 David Malcolm 2020-10-14 17:16:42 UTC
This is probably implementable as a -fanalyzer warning.
Comment 4 GCC Commits 2020-11-12 02:18:34 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:5e00ad3ffbfb4df7242c313a0d836f5b538eb2fb

commit r11-4930-g5e00ad3ffbfb4df7242c313a0d836f5b538eb2fb
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Nov 11 21:16:45 2020 -0500

    analyzer: warn on invalid shift counts [PR97424]
    
    This patch implements -Wanalyzer-shift-count-negative
    and -Wanalyzer-shift-count-overflow, analogous to the C/C++
    warnings -Wshift-count-negative and -Wshift-count-overflow, but
    implemented via interprocedural path analysis rather than via parsing
    in a front end, and thus capable of detecting interprocedural cases that the
    warnings implemented in the front ends can miss.
    
    gcc/analyzer/ChangeLog:
            PR tree-optimization/97424
            * analyzer.opt (Wanalyzer-shift-count-negative): New.
            (Wanalyzer-shift-count-overflow): New.
            * region-model.cc (class shift_count_negative_diagnostic): New.
            (class shift_count_overflow_diagnostic): New.
            (region_model::get_gassign_result): Complain about shift counts that
            are negative or are >= the operand's type's width.
    
    gcc/ChangeLog:
            PR tree-optimization/97424
            * doc/invoke.texi (Static Analyzer Options): Add
            -Wno-analyzer-shift-count-negative and
            -Wno-analyzer-shift-count-overflow.
            (-Wno-analyzer-shift-count-negative): New.
            (-Wno-analyzer-shift-count-overflow): New.
    
    gcc/testsuite/ChangeLog:
            PR tree-optimization/97424
            * gcc.dg/analyzer/invalid-shift-1.c: New test.
Comment 5 David Malcolm 2020-11-12 14:03:54 UTC
The above commit implements it as an analyzer warning.  Should I close this out, or should we keep it open for the __builtin_warning approach?
Comment 6 Florian Weimer 2020-11-27 19:11:30 UTC
(In reply to David Malcolm from comment #5)
> The above commit implements it as an analyzer warning.  Should I close this
> out, or should we keep it open for the __builtin_warning approach?

Thanks for the analyzer warning. I think the __builtin_warning approach is very desirable here. To me, it looks like GCC already did all the work to figure out this undefined.
Comment 7 Vincent Lefèvre 2020-12-26 16:41:24 UTC
I get a false positive on "b + 1 >= 64 ? 0UL : 1UL << (b + 1)" with a 64-bit unsigned long. See PR98447.