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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
