<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Oct 13, 2020, at 12:11 PM, Gábor Márton <<a href="mailto:martongabesz@gmail.com" class="">martongabesz@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">> Do you think that will actually be useful to the static analyzer use-case? I'd expect that in most use-cases, you'd want to provide the changes via some mechanism other than searching for a file named "APINotes.apinotes" in the same directory as you found the header. Other than for Swift with Apple frameworks, it seems likely to be pretty rare that the producer of a particular library would be the same entity providing a file of extra attributes on the side?<br class=""><div class=""><br class=""></div><div class="">It might be convenient for a library author to attach a YAML file that describes which functions should be checked e.g. for taint analysis. This file could be placed conveniently right next to the headers. On the other hand, this is not an option for some system libraries (e.g. libc), we definitely don't want libc authors to provide this data, it should be the role of the analyzer developers. Actually we do that today for many standard C/C++ or POSIX functions, but we simply <a href="https://github.com/llvm/llvm-project/blob/08097fc6a9746370e073e617036847d9ea46c9f4/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp#L1778" class="">hardcode the information into the source code of the analyzer</a> itself.</div><div class=""><br class=""></div><div class=""><div class="">> I wouldn't think so, since this mechanism modifies the nodes as part of their initial creation, not as a subsequent mutation?</div><div class=""><br class=""></div><div class="">My understanding is that we create a Decl and then a bit later we create an Attr and attach that to it, so this seems to be a mutation to me. However, I am not sure, perhaps this should not be interpreted as such.</div></div></div></div></blockquote><div><br class=""></div><div><div style="margin: 0px; font-stretch: normal; line-height: normal;" class="">There seem to be three systematic places a consumer virtual could be called:</div>
<ol class="">
<li style="margin: 0px; font-stretch: normal; line-height: normal;" class="">Between Parser and Sema, s.t. the user could tweak e.g. the `ParsedAttributes` of a `Declarator D` before `Actions.ActOnDeclarator(D)` is called, or whatever. (Ugh.)</li>
<li style="margin: 0px; font-stretch: normal; line-height: normal;" class="">After Sema is finished acting on a node, but before subsequent parsing. I.e., on the Decl returned by e.g. `Actions.ActOnDeclarator()`.</li>
<ul class="">
<li style="margin: 0px; font-stretch: normal; line-height: normal;" class="">That *would* require a call to the ASTMutationListener. (Ugh.)</li>
</ul>
<li style="margin: 0px; font-stretch: normal; line-height: normal;" class="">Immediately after a node is allocated via `*::Create()` in `Sema::ActOn*` etc. </li>
<ul class="">
<li style="margin: 0px; font-stretch: normal; line-height: normal;" class="">I.e., these calls would be a bit before the present placement of most `ProcessAPINotes(D)` calls.</li></ul></ol><div style="margin: 0px; font-stretch: normal; line-height: normal;" class="">#3, James’s solution, seems ideal. Because Sema cannot possibly have "acted on"/depended on a node which has only just been allocated (right?), this would give the consumer a hook to both a) visit a node *along with* the Parser/Sema state info at the time it was created, and b) tweak how a `FooDecl` is parsed/instantiated before Sema can see it.</div><div style="margin: 0px; font-stretch: normal; line-height: normal;" class=""><br class=""></div><div style="margin: 0px; font-stretch: normal; line-height: normal;" class="">All this could be contained in a simple override of a virtual e.g. `VisitCreatedFooDecl(FooDecl *D, bool Parsed1Instantiated0)` exposed in ASTConsumer or ExternalSemaSource. </div><div style="margin: 0px; font-stretch: normal; line-height: normal; min-height: 14px;" class=""><br class=""></div><div style="margin: 0px; font-stretch: normal; line-height: normal;" class="">APINotes would presumably then be able to move all calls to `ProcessAPINotes(D)` out of Sema and into `VisitCreatedFunctionDecl(FunctionDecl *D)/VisitCreatedObjCMethodDecl(…)/VisitCreatedParmVarDecl(…)` etc. overrides in its consumer.</div><div style="margin: 0px; font-stretch: normal; line-height: normal; min-height: 14px;" class=""><br class=""></div><div style="margin: 0px; font-stretch: normal; line-height: normal;" class="">If all the premises hold, this would be a broad gain in functionality via a familiar RecursiveASTVisitor-like interface.</div></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail-yj6qo"></div><div class="gmail-adL"><br class=""></div></div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 12, 2020 at 9:30 PM James Y Knight <<a href="mailto:jyknight@google.com" class="">jyknight@google.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class=""><div dir="ltr" class="">On Mon, Oct 12, 2020 at 12:46 PM Gábor Márton <<a href="mailto:martongabesz@gmail.com" target="_blank" class="">martongabesz@gmail.com</a>> wrote:<br class=""></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class="">+1 for separating the Swift specific Sema logic the way you described.<br class=""><div class="">However, there are valuable implementations in <a href="https://github.com/apple/llvm-project/tree/apple/main/clang/lib/APINotes" target="_blank" class="">lib/APINotes/*</a> which we could use for a generic APINotes framework in Clang. For instance, <a href="https://github.com/apple/llvm-project/blob/apple/main/clang/lib/APINotes/APINotesManager.cpp#L325" target="_blank" class="">APINotesManager::findAPINotes</a> encapsulates the logic for finding the APINotes files, this seems to be totally independent from Swift.<br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">Do you think that will actually be useful to the static analyzer use-case? I'd expect that in most use-cases, you'd want to provide the changes via some mechanism other than searching for a file named "APINotes.apinotes" in the same directory as you found the header. Other than for Swift with Apple frameworks, it seems likely to be pretty rare that the producer of a particular library would be the same entity providing a file of extra attributes on the side?</div><div class=""><br class=""></div><div class="">Also, I believe that particular function is unused in Swift. AFAICT, Swift only uses the modules-based apinotes loading (loadCurrentModuleAPINotes), not the header-path-based loading (that is: it passes "-fapinotes-modules" to its internal Clang invocation, not "-fapinotes").</div><div class=""><br class=""></div><div class=""><div class=""></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class=""><div class="">A side note: <a href="https://clang.llvm.org/doxygen/classclang_1_1ASTMutationListener.html" target="_blank" class="">ASTMutationListener</a> is the interface which notifies any consumers when an AST node is changed after its creation. I think we should notify all clients once we add a new APINote Attribute (e.g. see <a href="https://clang.llvm.org/doxygen/classclang_1_1ASTMutationListener.html#abd04809c814e5ea2af681465004f32fe" target="_blank" class="">AddedAttributeToRecord</a>), shouldn't we?</div></div></blockquote><div class=""><br class=""></div><div class="">I wouldn't think so, since this mechanism modifies the nodes as part of their initial creation, not as a subsequent mutation?</div><div class=""><br class=""></div></div></div>
</blockquote></div>
</div></blockquote></div><br class=""></body></html>