CuriousGeorgiy added a comment. @steakhal
Thanks for the review comments! > This line should be just as long, as the line above. Fixed. > Our docs aren't great, but we should have a brief description what the > checker detects, basically here it would be "Find null pointers being passed > to printf-like functions. Usually, passing null pointers to such functions is > implementation defined, thus non-portable. Printf-like functions are: x, y, > z." > Illustrate one example for diagnosing a case. Also illustrate one case > similar to the bad one, that is fixed (basically ensure that the pointer is > not null there). Fixed. Added a generic comment about the checker collection, added a check list and described the printf pointer conversion specifier checker and provided TP and TN examples for it. > I don't think we should create a separate package. A separate checker should > be enough in `alpha.unix`, or `alpha.optin`, or just `optin`. Moved the checker to the `alpha.unix` package. > Usually, the user-facing checker name which is the <"..."> part there, > matches the name of the checker's class name. It helps for maintenance. > I would strongly suggest keeping this naming convention. Changed the checker name to `APIPortabilityMinor`, dropping the `Unix` prefix, which seems redundant as it is part of the checker path. > Usually, we put separate checkers into their own file. Fixed. > Prefer using the check::PreCall callback over the PreStmt. Fixed. > Nowdays, if I'm not mistaken, we just inline initialize these eagerly. > That way you could also avoid marking it mutable. Fixed. > Feel free to just call it reportBug. I cannot call it this way, since it is bug reporting function for the printf family pointer conversion specifier specific check. > Also, in the cpp file, usually we use the namespaces clang,llvm,ento already. > You don't need to fully-qualify these names. Fixed. Though, AFAIC, I still need to qualify the `register` and `shouldRegister` functions with the `ento` namespace, otherwise the ADL fails. > For matching names, we usually use `CallDescriptions` to do it for us. > If think you could use them while also checking if the Call is a > `CallEvent::isGlobalCFunction()`. I suspect, you only wanna check those. > I think you should define a `CallDescriptionMap`, because you will also need > your specific data_arg_index... > Look for uses of such maps and you will get inspired. Thanks a lot for pointing this out, it looks very neat indeed, applied your suggestion, and the checker looks a lot cleaner now. > I'd highly recommend not touching this file to avoid the bulk change in the > expected plist output. Please just introduce a new test file. Fixed. > I would appreciate a test demonstrating that the pointer argument won't get > constrained after the printf call. I think such case was already discussed. > We should also demonstrate that unknown pointers (which doesn't known to be > neither null, nor non-null) won't raise issues. Added these test cases to the `printf_pointer_conversion_specifier_null_pointer_constraints` test. > In the checker-logic, we have the `data_args_index` for each printf-like > function. > Can you demonstrate that each is handled as expected? AFAIC, I have already done this in the `test_printf_pointer_conversion_specifier_null_various_arguments` test. > Also have a test when the formatstring itself is null. AFAIC, passing a null format string is already UB, so the check shouldn't handle this case specially. What test would you like to see? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154838/new/ https://reviews.llvm.org/D154838 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
