[llvm] [InstCombine] Support gep nuw in icmp folds (PR #118472)

Nico Weber via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 17 15:49:39 PST 2024


nico wrote:

Is this the only recent change that allows LLVM to make assumptions about pointer overflows?

+1 to this being a problem. If we update clang in chromium, we pick this up and potentially introduce all kinds of silent security vulnerabilities.  https://crbug.com/384391188 is an example where this erased a security pointer check. The check was technically invalid, but it worked prior to this, and after this it's an OOB write instead. And that's a case where we happened to have a test – several other places don't have a check probably.

Another idea: C++ made signed integer overflow defined recently as far as I know, and unsigned integer overflow is also defined. Maybe it'd make sense to give clang a flag that says "give pointer overflow defined two's complement semantics"? Does `-fwrapv-pointer` do this? Maybe we can just add support for that to clang? Then people could use that flag to buy themselves time to audit their codebase, or to keep this off indefinitely.

Failing that, if this is the only patch, could we get a (possibly temporary) mllvm flag for turning this off?

One reason why this is a bit of a problem: #116871 made -Wunused-private-field stricter, in a way that's not easy to put behind a new warning flag. It makes that warning fire a lot more. We cleaned up our codebase (required updating ~80 files) to  be clean with the new warning, and we now have these options:

1. Do not update clang while the discussion here is ongoing. Pinned clang wont find new -Wunused-private-field instances, so this'll create work for us when we update next. (There's also a similar situation for -Wreturn-stack-address – https://crbug.com/384033056 – but that one fires much less often and it's less of a problem.)

2. Update clang, make sure the warnings stay fixed, but potentially pick up all kinds of silent new security vulnerabilities due to this patch.

Neither alternative is particularly great. If there was a way to update but turn off this here for a while, that'd give us a better path forward.

https://github.com/llvm/llvm-project/pull/118472


More information about the llvm-commits mailing list