[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

Balazs Benics via cfe-commits cfe-commits at lists.llvm.org
Sat Feb 3 00:05:44 PST 2024


steakhal wrote:

> The code LGTM with some minor remarks. (Disclaimer: I'm not familiar with these MS functions.)
> 
> I'm not sure whether these "builtin by Microsoft" functions are in scope for "our" BuiltinFunctionChecker which previously only checked functions that are recognized as `Builtin::BI__something` by Clang. (However, I can believe that there is no better place for them and I don't think that they would cause problems here.)

For me, this file would be where I would look to see how `__assume` is eval called. This is actually an unknown function in regular mode, but with `-fms-extensions` it's actually defined. Check the AST for this [example](https://godbolt.org/z/1xdbP8scx). So, techniquely, it's not a builtin, but feels like it.

About the remarks, yes, let's do [Clean As You Code](https://docs.sonarsource.com/sonarqube/latest/user-guide/clean-as-you-code/#what-is-clean-as-you-code)! I'm all in.


> Hey!
> 
> Thanks for looking into this!
> 
> Did you actually encounter this call in the wild? The reason I ask, because their definition looks like this in the current version of `sal.h`:
> 
> ```
> #ifndef __analysis_assume // [
> #ifdef _PREFAST_ // [
> #define __analysis_assume(expr) __assume(expr)
> #else // ][
> #define __analysis_assume(expr)
> #endif // ]
> #endif // ]
> 
> #ifndef _Analysis_assume_ // [
> #ifdef _PREFAST_ // [
> #define _Analysis_assume_(expr) __assume(expr)
> #else // ][
> #define _Analysis_assume_(expr)
> #endif // ]
> #endif // ]
> ```
> 
> The basic idea is, when MSVC's analyzer is invoked, `_PREFAST_` is defined, and these macros should expand to `__assume(expr)`. This makes me a bit surprised if the original names are present in the preprocessed code.
> 
> There is no difference between `__analysis_assume` and `_Analysis_assume_`. The former is following the naming conventions in SAL 1, the latter is following the conventions of SAL 2. The latter is preferred in user code, but MSVC still supports both spellings.

I think I've seen FPs on ChakraCore and on the DotnetRuntime and on other Windows-related projects defining some of their assert macros by wrapping `__analysis_assume`. I can't exactly recall ATM where exactly I've seen that, but I'll do a measurment to confirm that this PR actually improves the situation.

Anyways, it turns out the FP I wanted to fix actually expands into using `__builtin_trap()`, which is still not modeled in CSA :face_with_spiral_eyes: I'll create a separate PR for handling that.

I think this PR stands on it's own, and improves the situation - (I'll check and come back of course).
I hope `__assume()` takes a single parameter (of any type) and returns `void` though.

Sorry for not doing my homework in the beginning, and thanks for catching that!
I though it's gonna be an easy one and I fire off a PR on my tea-break.

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


More information about the cfe-commits mailing list