[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)

Artem Dergachev via cfe-commits cfe-commits at lists.llvm.org
Mon May 13 15:48:48 PDT 2024


haoNoQ wrote:

Hi! Thank you for digging into this! Sorry for the delay.

> The new UnsafeBufferUsageCtorAttrGadget gadget explicitly avoids matching against the std::span(ptr, size) constructor because that is handled by SpanTwoParamConstructorGadget and we never want two gadgets to match the same thing (and this is guarded by asserts).

Hmm at a glance I'm not sure this should really be illegal. It's a big problem when fixable gadgets overlap, because this means that they'll try to fix the same code in two incompatible ways. I don't immediately see why this would be a problem for warning gadgets that don't try to fix anything. This just means that the code is non-compliant for two different reasons, which is theoretically fine. I also tried to reproduce your assert and I didn't hit it; which one are you running into?

> To handle this we allow the gadget to control if the warning is general (it calls handleUnsafeBufferUsage()) or is a std-container-specific warning (it calls handleUnsafeOperationInContainer()).

Ooo I love this.

My initial thinking was, just make the base Gadget class "public" (move it into `UnsafeBufferUsage.h` so that the handler could see it) and make the handler switch over the gadget's `getKind()` to produce specialized messages for each gadget. This would help us unscrew the rest of the `Reporter` code so that it also didn't have to guess by statement kind.

Your design can grow into that too - make a separate handler method for each gadget kind (or group of kinds), and it preserves even more encapsulation. Additionally it throws away `getBaseStmt()` in favor of a more "integrated" solution. I'm a big fan, let's keep this.

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


More information about the cfe-commits mailing list