Adding Support for APINotes

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Adding Support for APINotes

Manas via cfe-dev
Hi!

I'd like to revive the effort around merging APINotes support into the llvm.org repository.  This was previously discussed at http://lists.llvm.org/pipermail/cfe-dev/2017-May/053860.html nearly 3 years ago.  The overall consensus seemed neutral to slightly positive.  Now that the Swift specific attributes have been merged, the APINotes seem like a good next step towards converging the fork.

I've put up https://reviews.llvm.org/D88446 to add initial documentation on the feature before trying to add the actual implementation with the goal of gathering commits on the technical aspects of the feature.

Thanks.

--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
Hi Saleem,

About the technical aspects. The feature to be able to add extra information to library functions would be really helpful in the Clang Static Analyzer. APINotes might be a way forward to achieve that.

We'd like to add the following to functions
  • Argument constraints
    • Not null
    • Range, e.g. isalpha(int x) x must be in [0,255].
    • Buffer size, e.g. fread(x1, x2, x3)  x1 is a buffer with a size denoted by x2 * x3
  • Taint rules
    • propagation 
          int x; // x is tainted
        int y;
        myPropagator(x, &y); // y is tainted
    • filter
          int x; // x is tainted
        isOutOfRange(&x); // x is not tainted anymore

    • sink
          int x; // x is tainted
        myNamespace::mySink(x); // It will warn

  • Error return rules, e.g. the return value of mktime() should always be checked against -1.  (D72705)
  • Indicate which functions we should consider with statistical checkers (WIP, not upstreamed yet).
Note to taint analysis: we might want to tag global variables as sources. E.g. std::cin.

I think that we could extend the APINote implementation that you have in the referenced commit, but with a CSA specific attribute:
def CSANotes : InheritableAttr {
  let Spellings = [GNU<"csa">];
  let Args = [StringArgument<"FreeFormatOrSomeYamlMaybe">];
  let Subjects = SubjectList<[Tag, Function, Var],
                             ErrorDiag>;
}
The string argument could be parsed specifically to the CSA developers' needs in a CSA specific implementation file. IMHO it would be a flexible solution because we could avoid adding many CSA specific attributes to Attrs.td and it would allow us to make experiments.

My concerns about the referenced APINotes implementation:
  • Specifically with the `'Name' key`. How could we match in C++
    • overloaded functions
    • member functions
    • functions in namespaces
    • function templates
    • member functions of class templates (e.g. std::vector::begin)
  • Do we have a mechanism to test/indicate if a note is not applied but it should have been?

Adding CSA guys who might want to comment.

Thanks,
Gabor

On Mon, Sep 28, 2020 at 11:10 PM Saleem Abdulrasool via cfe-dev <[hidden email]> wrote:
Hi!

I'd like to revive the effort around merging APINotes support into the llvm.org repository.  This was previously discussed at http://lists.llvm.org/pipermail/cfe-dev/2017-May/053860.html nearly 3 years ago.  The overall consensus seemed neutral to slightly positive.  Now that the Swift specific attributes have been merged, the APINotes seem like a good next step towards converging the fork.

I've put up https://reviews.llvm.org/D88446 to add initial documentation on the feature before trying to add the actual implementation with the goal of gathering commits on the technical aspects of the feature.

Thanks.

--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
Static analyzer applications are indeed a long-term dream, cf. http://lists.llvm.org/pipermail/cfe-dev/2015-December/046354.html - We're super interested in attaching arbitrary annotations to functions in headers that we can't change directly, or possibly even let our users inject annotations into headers they have to use but can't change.

On 9/29/20 9:11 AM, Gábor Márton wrote:
Hi Saleem,

About the technical aspects. The feature to be able to add extra information to library functions would be really helpful in the Clang Static Analyzer. APINotes might be a way forward to achieve that.

We'd like to add the following to functions
  • Argument constraints
    • Not null
    • Range, e.g. isalpha(int x) x must be in [0,255].
    • Buffer size, e.g. fread(x1, x2, x3)  x1 is a buffer with a size denoted by x2 * x3
  • Taint rules
    • propagation 
          int x; // x is tainted
        int y;
        myPropagator(x, &y); // y is tainted
    • filter
          int x; // x is tainted
        isOutOfRange(&x); // x is not tainted anymore

    • sink
          int x; // x is tainted
        myNamespace::mySink(x); // It will warn

  • Error return rules, e.g. the return value of mktime() should always be checked against -1.  (D72705)
  • Indicate which functions we should consider with statistical checkers (WIP, not upstreamed yet).
Note to taint analysis: we might want to tag global variables as sources. E.g. std::cin.

I think that we could extend the APINote implementation that you have in the referenced commit, but with a CSA specific attribute:
def CSANotes : InheritableAttr {
  let Spellings = [GNU<"csa">];
  let Args = [StringArgument<"FreeFormatOrSomeYamlMaybe">];
  let Subjects = SubjectList<[Tag, Function, Var],
                             ErrorDiag>;
}
The string argument could be parsed specifically to the CSA developers' needs in a CSA specific implementation file. IMHO it would be a flexible solution because we could avoid adding many CSA specific attributes to Attrs.td and it would allow us to make experiments.

My concerns about the referenced APINotes implementation:
  • Specifically with the `'Name' key`. How could we match in C++
    • overloaded functions
    • member functions
    • functions in namespaces
    • function templates
    • member functions of class templates (e.g. std::vector::begin)
  • Do we have a mechanism to test/indicate if a note is not applied but it should have been?

Adding CSA guys who might want to comment.

Thanks,
Gabor

On Mon, Sep 28, 2020 at 11:10 PM Saleem Abdulrasool via cfe-dev <[hidden email]> wrote:
Hi!

I'd like to revive the effort around merging APINotes support into the llvm.org repository.  This was previously discussed at http://lists.llvm.org/pipermail/cfe-dev/2017-May/053860.html nearly 3 years ago.  The overall consensus seemed neutral to slightly positive.  Now that the Swift specific attributes have been merged, the APINotes seem like a good next step towards converging the fork.

I've put up https://reviews.llvm.org/D88446 to add initial documentation on the feature before trying to add the actual implementation with the goal of gathering commits on the technical aspects of the feature.

Thanks.

--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
In reply to this post by Manas via cfe-dev
Hi Gábor,

The C++ name-matching I think is an interesting case.  I believe that at the moment this is beyond the scope of the initial implementation that I am driving with.  I don't think that it is beyond extension though.  The names actually are matched using the spelling at the site of the declaration.  I would imagine that you could do the same thing and check if the names match, which actually is good because that means that you do not need to worry about the decorated names, but rather the names as spelt by the user.  This will likely have some amount of work involved as one of the interesting cases that comes to mind is the inline namespaces.  I don't think that the design here precludes the functionality, merely that we will need additional processing specifically for C++ (which I think is reasonable).

On Tue, Sep 29, 2020 at 9:11 AM Gábor Márton <[hidden email]> wrote:
Hi Saleem,

About the technical aspects. The feature to be able to add extra information to library functions would be really helpful in the Clang Static Analyzer. APINotes might be a way forward to achieve that.

We'd like to add the following to functions
  • Argument constraints
    • Not null
    • Range, e.g. isalpha(int x) x must be in [0,255].
    • Buffer size, e.g. fread(x1, x2, x3)  x1 is a buffer with a size denoted by x2 * x3
  • Taint rules
    • propagation 
          int x; // x is tainted
        int y;
        myPropagator(x, &y); // y is tainted
    • filter
          int x; // x is tainted
        isOutOfRange(&x); // x is not tainted anymore

    • sink
          int x; // x is tainted
        myNamespace::mySink(x); // It will warn

  • Error return rules, e.g. the return value of mktime() should always be checked against -1.  (D72705)
  • Indicate which functions we should consider with statistical checkers (WIP, not upstreamed yet).
Note to taint analysis: we might want to tag global variables as sources. E.g. std::cin.

I think that we could extend the APINote implementation that you have in the referenced commit, but with a CSA specific attribute:
def CSANotes : InheritableAttr {
  let Spellings = [GNU<"csa">];
  let Args = [StringArgument<"FreeFormatOrSomeYamlMaybe">];
  let Subjects = SubjectList<[Tag, Function, Var],
                             ErrorDiag>;
}
The string argument could be parsed specifically to the CSA developers' needs in a CSA specific implementation file. IMHO it would be a flexible solution because we could avoid adding many CSA specific attributes to Attrs.td and it would allow us to make experiments.

My concerns about the referenced APINotes implementation:
  • Specifically with the `'Name' key`. How could we match in C++
    • overloaded functions
    • member functions
    • functions in namespaces
    • function templates
    • member functions of class templates (e.g. std::vector::begin)
  • Do we have a mechanism to test/indicate if a note is not applied but it should have been?

Adding CSA guys who might want to comment.

Thanks,
Gabor

On Mon, Sep 28, 2020 at 11:10 PM Saleem Abdulrasool via cfe-dev <[hidden email]> wrote:
Hi!

I'd like to revive the effort around merging APINotes support into the llvm.org repository.  This was previously discussed at http://lists.llvm.org/pipermail/cfe-dev/2017-May/053860.html nearly 3 years ago.  The overall consensus seemed neutral to slightly positive.  Now that the Swift specific attributes have been merged, the APINotes seem like a good next step towards converging the fork.

I've put up https://reviews.llvm.org/D88446 to add initial documentation on the feature before trying to add the actual implementation with the goal of gathering commits on the technical aspects of the feature.

Thanks.

--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
In reply to this post by Manas via cfe-dev
Every time this discussion has recurred, people have noted that a feature like this could be valuable for many use-cases beyond swift. And I don't disagree. But, what we actually have here is far from a generic "extra API information" solution. It has a lot of missing functionality, without even a clear path towards adding it. And, many of the features which it does have are extremely swift-specific. What has been built here is clearly purpose-designed just for Swift/ObjC-interop, and it does not really seem realistic to me to expect this to be able to accomplish people's wider dreams of a generally-useful API sidecar mechanism.

One of the clearest examples of a feature which is strikingly out-of-place as an upstream feature is the "SwiftVersions" stanza, and correspondingly, -fapi-notes-swift-version command-line argument -- but that's hardly the only such issue. I could come up with ideas on how to avoid needing such a weird thing, but I think that would probably not be productive.

Because, I think the question which is being actually being implicitly asked in this thread is:
  Is the Clang community OK with adding this (hefty!) amount of code to support a feature which is currently -- and most likely will remain -- only useful for Swift? Furthermore, is it OK to add essentially as-is, with no ability to significantly affect the design? Incompatible changes would then break the deployed Swift usage, and therefore cannot be accepted.

Quite possibly the answer to both is indeed "yes". Personally, I am vaguely uncomfortable with this, but not entirely opposed to including it on those grounds.

Yet, what I definitely do not want to see is this getting pushed upstream _without_ it explicitly being stated that the above is what's happening. We need to acknowledge that what is actually being proposed here is "SwiftAPINotes", not "APINotes", and for people to be OK with that.

(And, as a sidenote, it also seems unclear to me that Swift really needed APINotes to be put into Clang (even Swift's fork of Clang) vs building the similar functionality into the Swift consumer of the Clang AST. Possibly this was an arbitrary choice made early on. Or maybe there's good reasons why it wouldn't work to do that way which aren't obvious at a quick glance. Bu, I suspect there is no appetite for making such a large design change now, even if it could've been better, so that's not really a useful discussion to have.)

On Mon, Sep 28, 2020 at 5:10 PM Saleem Abdulrasool via cfe-dev <[hidden email]> wrote:
Hi!

I'd like to revive the effort around merging APINotes support into the llvm.org repository.  This was previously discussed at http://lists.llvm.org/pipermail/cfe-dev/2017-May/053860.html nearly 3 years ago.  The overall consensus seemed neutral to slightly positive.  Now that the Swift specific attributes have been merged, the APINotes seem like a good next step towards converging the fork.

I've put up https://reviews.llvm.org/D88446 to add initial documentation on the feature before trying to add the actual implementation with the goal of gathering commits on the technical aspects of the feature.

Thanks.

--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
On Oct 6, 2020, at 6:39 AM, James Y Knight via cfe-dev <[hidden email]> wrote:

> Every time this discussion has recurred, people have noted that a feature like this could be valuable for many use-cases beyond swift. And I don't disagree. But, what we actually have here is far from a generic "extra API information" solution. It has a lot of missing functionality, without even a clear path towards adding it. And, many of the features which it does have are extremely swift-specific. What has been built here is clearly purpose-designed just for Swift/ObjC-interop, and it does not really seem realistic to me to expect this to be able to accomplish people's wider dreams of a generally-useful API sidecar mechanism.
>
> One of the clearest examples of a feature which is strikingly out-of-place as an upstream feature is the "SwiftVersions" stanza, and correspondingly, -fapi-notes-swift-version command-line argument -- but that's hardly the only such issue. I could come up with ideas on how to avoid needing such a weird thing, but I think that would probably not be productive.
>
> Because, I think the question which is being actually being implicitly asked in this thread is:
>   Is the Clang community OK with adding this (hefty!) amount of code to support a feature which is currently -- and most likely will remain -- only useful for Swift? Furthermore, is it OK to add essentially as-is, with no ability to significantly affect the design? Incompatible changes would then break the deployed Swift usage, and therefore cannot be accepted.
>
> Quite possibly the answer to both is indeed "yes". Personally, I am vaguely uncomfortable with this, but not entirely opposed to including it on those grounds.
>
> Yet, what I definitely do not want to see is this getting pushed upstream _without_ it explicitly being stated that the above is what's happening. We need to acknowledge that what is actually being proposed here is "SwiftAPINotes", not "APINotes", and for people to be OK with that.
>
> (And, as a sidenote, it also seems unclear to me that Swift really needed APINotes to be put into Clang (even Swift's fork of Clang) vs building the similar functionality into the Swift consumer of the Clang AST. Possibly this was an arbitrary choice made early on. Or maybe there's good reasons why it wouldn't work to do that way which aren't obvious at a quick glance. Bu, I suspect there is no appetite for making such a large design change now, even if it could've been better, so that's not really a useful discussion to have.)

I think that is a fairly reasonable characterization from what I know, but I would go a bit further - we don’t always make “perfection” be the barrier to landing something in mainline;  we make “good enough” be the threshold.  Is the current implementation of APINotes a useful baseline that can be extended in the future?

-Chris
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev


On Oct 6, 2020, at 10:10 PM, Chris Lattner <[hidden email]> wrote:

(And, as a sidenote, it also seems unclear to me that Swift really needed APINotes to be put into Clang (even Swift's fork of Clang) vs building the similar functionality into the Swift consumer of the Clang AST. Possibly this was an arbitrary choice made early on. Or maybe there's good reasons why it wouldn't work to do that way which aren't obvious at a quick glance. Bu, I suspect there is no appetite for making such a large design change now, even if it could've been better, so that's not really a useful discussion to have.)

I think that is a fairly reasonable characterization from what I know, but I would go a bit further - we don’t always make “perfection” be the barrier to landing something in mainline;  we make “good enough” be the threshold.  Is the current implementation of APINotes a useful baseline that can be extended in the future?

Just to be pointed about this, I’ll give an example: modules.  Modules was built specifically to enable Swift, but was explained as a way of accelerating C/ObjC build times.  Clang supported C++ at the time, but modules did not.  I think that modules existing in clang helped catalyze and provide a basis for C++ modules that has benefited many in the community, even though apple didn’t build it.

Would it make sense to hold APINotes to a similar bar as Clang Modules originally were?

-Chris

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
In reply to this post by Manas via cfe-dev


On Oct 6, 2020, at 9:39 AM, James Y Knight via cfe-dev <[hidden email]> wrote:

Every time this discussion has recurred, people have noted that a feature like this could be valuable for many use-cases beyond swift. And I don't disagree. But, what we actually have here is far from a generic "extra API information" solution. It has a lot of missing functionality, without even a clear path towards adding it. And, many of the features which it does have are extremely swift-specific. What has been built here is clearly purpose-designed just for Swift/ObjC-interop, and it does not really seem realistic to me to expect this to be able to accomplish people's wider dreams of a generally-useful API sidecar mechanism.

One of the clearest examples of a feature which is strikingly out-of-place as an upstream feature is the "SwiftVersions" stanza, and correspondingly, -fapi-notes-swift-version command-line argument -- but that's hardly the only such issue. I could come up with ideas on how to avoid needing such a weird thing, but I think that would probably not be productive.

Because, I think the question which is being actually being implicitly asked in this thread is:
  Is the Clang community OK with adding this (hefty!) amount of code to support a feature which is currently -- and most likely will remain -- only useful for Swift? Furthermore, is it OK to add essentially as-is, with no ability to significantly affect the design? Incompatible changes would then break the deployed Swift usage, and therefore cannot be accepted.

Quite possibly the answer to both is indeed "yes". Personally, I am vaguely uncomfortable with this, but not entirely opposed to including it on those grounds.

Yet, what I definitely do not want to see is this getting pushed upstream _without_ it explicitly being stated that the above is what's happening. We need to acknowledge that what is actually being proposed here is "SwiftAPINotes", not "APINotes", and for people to be OK with that.

(And, as a sidenote, it also seems unclear to me that Swift really needed APINotes to be put into Clang (even Swift's fork of Clang) vs building the similar functionality into the Swift consumer of the Clang AST. Possibly this was an arbitrary choice made early on. Or maybe there's good reasons why it wouldn't work to do that way which aren't obvious at a quick glance. Bu, I suspect there is no appetite for making such a large design change now, even if it could've been better, so that's not really a useful discussion to have.)

IIUC APINotes wants to add attributes during parsing so that the those attributes are then treated just like real parsed attributes would be by Sema.  

Ideally this sort of thing should be able to be contained in an ASTConsumer, but it presently cannot: the only real option ASTConsumer provides is to traverse the AST post-parsing in an override of `HandleTranslationUnit()`.  This leaves APINotes no choice but to add its details directly to Sema and Parser, with the result that Swift-specific details would necessarily show up in the guts of mainline clang were this to be upstreamed.  

If ASTConsumer instead provided a comprehensive virtual interface (but with negligible overhead, somehow) to perform custom consumer actions at strategic points during parsing/Sema (e.g., call consumer virtuals in each Sema::ActOn*), this conflict probably could be, or at least could have been, avoided.

Note too that this issue resonates with one a user faced just the other day: he wanted to call `Sema::LookupName` within an ASTMatchers callback (during HandleTranslationUnit, as usual) to determine if a certain node could be rewritten w/o changing its semantics — but of course the necessary Sema/Parser scope info is long gone by that point.  If only he had the option to perform his logic on the node immediately after it was parsed, there would be no issue. Because he cannot, his only option was to laboriously recreate that state data in order to call LookupName.

In sum, the dearth of ASTConsumer virtuals hinder encapsulation, causing 1) Sema and Parser to be polluted with what should be a consumer’s details, and 2) consumers to be polluted with what should be Sema & Parser details, and making it difficult to upstream useful tools.


On Mon, Sep 28, 2020 at 5:10 PM Saleem Abdulrasool via cfe-dev <[hidden email]> wrote:
Hi!

I'd like to revive the effort around merging APINotes support into the llvm.org repository.  This was previously discussed at http://lists.llvm.org/pipermail/cfe-dev/2017-May/053860.html nearly 3 years ago.  The overall consensus seemed neutral to slightly positive.  Now that the Swift specific attributes have been merged, the APINotes seem like a good next step towards converging the fork.

I've put up https://reviews.llvm.org/D88446 to add initial documentation on the feature before trying to add the actual implementation with the goal of gathering commits on the technical aspects of the feature.

Thanks.

--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
In reply to this post by Manas via cfe-dev
Hi James,

Yes, I agree that the initial consumer of this functionality will be Swift.  However, the functionality itself should not be considered entirely specific to Swift.  Although I do not plan on currently extending it, patches to extend it beyond just Swift and adding new consumers would be welcome.

On Tue, Oct 6, 2020 at 6:39 AM James Y Knight <[hidden email]> wrote:
Every time this discussion has recurred, people have noted that a feature like this could be valuable for many use-cases beyond swift. And I don't disagree. But, what we actually have here is far from a generic "extra API information" solution. It has a lot of missing functionality, without even a clear path towards adding it. And, many of the features which it does have are extremely swift-specific. What has been built here is clearly purpose-designed just for Swift/ObjC-interop, and it does not really seem realistic to me to expect this to be able to accomplish people's wider dreams of a generally-useful API sidecar mechanism.

One of the clearest examples of a feature which is strikingly out-of-place as an upstream feature is the "SwiftVersions" stanza, and correspondingly, -fapi-notes-swift-version command-line argument -- but that's hardly the only such issue. I could come up with ideas on how to avoid needing such a weird thing, but I think that would probably not be productive.

Because, I think the question which is being actually being implicitly asked in this thread is:
  Is the Clang community OK with adding this (hefty!) amount of code to support a feature which is currently -- and most likely will remain -- only useful for Swift? Furthermore, is it OK to add essentially as-is, with no ability to significantly affect the design? Incompatible changes would then break the deployed Swift usage, and therefore cannot be accepted.

Quite possibly the answer to both is indeed "yes". Personally, I am vaguely uncomfortable with this, but not entirely opposed to including it on those grounds.

Yet, what I definitely do not want to see is this getting pushed upstream _without_ it explicitly being stated that the above is what's happening. We need to acknowledge that what is actually being proposed here is "SwiftAPINotes", not "APINotes", and for people to be OK with that.

(And, as a sidenote, it also seems unclear to me that Swift really needed APINotes to be put into Clang (even Swift's fork of Clang) vs building the similar functionality into the Swift consumer of the Clang AST. Possibly this was an arbitrary choice made early on. Or maybe there's good reasons why it wouldn't work to do that way which aren't obvious at a quick glance. Bu, I suspect there is no appetite for making such a large design change now, even if it could've been better, so that's not really a useful discussion to have.)

On Mon, Sep 28, 2020 at 5:10 PM Saleem Abdulrasool via cfe-dev <[hidden email]> wrote:
Hi!

I'd like to revive the effort around merging APINotes support into the llvm.org repository.  This was previously discussed at http://lists.llvm.org/pipermail/cfe-dev/2017-May/053860.html nearly 3 years ago.  The overall consensus seemed neutral to slightly positive.  Now that the Swift specific attributes have been merged, the APINotes seem like a good next step towards converging the fork.

I've put up https://reviews.llvm.org/D88446 to add initial documentation on the feature before trying to add the actual implementation with the goal of gathering commits on the technical aspects of the feature.

Thanks.

--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
In reply to this post by Manas via cfe-dev


On Fri, Oct 9, 2020 at 10:21 AM David Rector <[hidden email]> wrote:
IIUC APINotes wants to add attributes during parsing so that the those attributes are then treated just like real parsed attributes would be by Sema.  

Ideally this sort of thing should be able to be contained in an ASTConsumer, but it presently cannot: the only real option ASTConsumer provides is to traverse the AST post-parsing in an override of `HandleTranslationUnit()`.  This leaves APINotes no choice but to add its details directly to Sema and Parser, with the result that Swift-specific details would necessarily show up in the guts of mainline clang were this to be upstreamed.  

If ASTConsumer instead provided a comprehensive virtual interface (but with negligible overhead, somehow) to perform custom consumer actions at strategic points during parsing/Sema (e.g., call consumer virtuals in each Sema::ActOn*), this conflict probably could be, or at least could have been, avoided.

Note too that this issue resonates with one a user faced just the other day: he wanted to call `Sema::LookupName` within an ASTMatchers callback (during HandleTranslationUnit, as usual) to determine if a certain node could be rewritten w/o changing its semantics — but of course the necessary Sema/Parser scope info is long gone by that point.  If only he had the option to perform his logic on the node immediately after it was parsed, there would be no issue. Because he cannot, his only option was to laboriously recreate that state data in order to call LookupName.

In sum, the dearth of ASTConsumer virtuals hinder encapsulation, causing 1) Sema and Parser to be polluted with what should be a consumer’s details, and 2) consumers to be polluted with what should be Sema & Parser details, and making it difficult to upstream useful tools.

This is an excellent point!

It looks like the primary change in Clang which is required for APINotes is to place "ProcessAPINotes" calls into each of the Sema::ActOn* functions which creates a (subclass of) Decl. This needs to occur after creating the Decl object and processing the attributes in the source, but prior to actually acting on it (e.g. see apple's SemaDeclObjC.cpp). This call could instead be a call to a generic hook for modifying the decl, rather than specifically for processing API notes. We already even have a container for such hooks in Sema, "ExternalSemaSource", which seems like a reasonable place to add a virtual method which may modify newly-created Decls.

If such a hook mechanism is added, it then seems entirely plausible that SemaAPINotes.cpp and lib/APINotes/* can be moved from the clang-fork into swift itself without requiring significant modifications of the code, or any change in functionality of the feature.

If this works out how it seems like it could, this seems like it may even be a better outcome for all parties (both the potential static analysis tooling use-cases, and for Swift).

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
+1 for separating the Swift specific Sema logic the way you described.
However, there are valuable implementations in lib/APINotes/* which we could use for a generic APINotes framework in Clang. For instance, APINotesManager::findAPINotes encapsulates the logic for finding the APINotes files, this seems to be totally independent from Swift.
A side note: ASTMutationListener 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 AddedAttributeToRecord), shouldn't we?

Gabor

On Mon, Oct 12, 2020 at 5:51 PM James Y Knight via cfe-dev <[hidden email]> wrote:


On Fri, Oct 9, 2020 at 10:21 AM David Rector <[hidden email]> wrote:
IIUC APINotes wants to add attributes during parsing so that the those attributes are then treated just like real parsed attributes would be by Sema.  

Ideally this sort of thing should be able to be contained in an ASTConsumer, but it presently cannot: the only real option ASTConsumer provides is to traverse the AST post-parsing in an override of `HandleTranslationUnit()`.  This leaves APINotes no choice but to add its details directly to Sema and Parser, with the result that Swift-specific details would necessarily show up in the guts of mainline clang were this to be upstreamed.  

If ASTConsumer instead provided a comprehensive virtual interface (but with negligible overhead, somehow) to perform custom consumer actions at strategic points during parsing/Sema (e.g., call consumer virtuals in each Sema::ActOn*), this conflict probably could be, or at least could have been, avoided.

Note too that this issue resonates with one a user faced just the other day: he wanted to call `Sema::LookupName` within an ASTMatchers callback (during HandleTranslationUnit, as usual) to determine if a certain node could be rewritten w/o changing its semantics — but of course the necessary Sema/Parser scope info is long gone by that point.  If only he had the option to perform his logic on the node immediately after it was parsed, there would be no issue. Because he cannot, his only option was to laboriously recreate that state data in order to call LookupName.

In sum, the dearth of ASTConsumer virtuals hinder encapsulation, causing 1) Sema and Parser to be polluted with what should be a consumer’s details, and 2) consumers to be polluted with what should be Sema & Parser details, and making it difficult to upstream useful tools.

This is an excellent point!

It looks like the primary change in Clang which is required for APINotes is to place "ProcessAPINotes" calls into each of the Sema::ActOn* functions which creates a (subclass of) Decl. This needs to occur after creating the Decl object and processing the attributes in the source, but prior to actually acting on it (e.g. see apple's SemaDeclObjC.cpp). This call could instead be a call to a generic hook for modifying the decl, rather than specifically for processing API notes. We already even have a container for such hooks in Sema, "ExternalSemaSource", which seems like a reasonable place to add a virtual method which may modify newly-created Decls.

If such a hook mechanism is added, it then seems entirely plausible that SemaAPINotes.cpp and lib/APINotes/* can be moved from the clang-fork into swift itself without requiring significant modifications of the code, or any change in functionality of the feature.

If this works out how it seems like it could, this seems like it may even be a better outcome for all parties (both the potential static analysis tooling use-cases, and for Swift).
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
On Mon, Oct 12, 2020 at 12:46 PM Gábor Márton <[hidden email]> wrote:
+1 for separating the Swift specific Sema logic the way you described.
However, there are valuable implementations in lib/APINotes/* which we could use for a generic APINotes framework in Clang. For instance, APINotesManager::findAPINotes encapsulates the logic for finding the APINotes files, this seems to be totally independent from Swift.

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?

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").

A side note: ASTMutationListener 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 AddedAttributeToRecord), shouldn't we?

I wouldn't think so, since this mechanism modifies the nodes as part of their initial creation, not as a subsequent mutation?


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
> 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?

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 hardcode the information into the source code of the analyzer itself.

> I wouldn't think so, since this mechanism modifies the nodes as part of their initial creation, not as a subsequent mutation?

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.


On Mon, Oct 12, 2020 at 9:30 PM James Y Knight <[hidden email]> wrote:
On Mon, Oct 12, 2020 at 12:46 PM Gábor Márton <[hidden email]> wrote:
+1 for separating the Swift specific Sema logic the way you described.
However, there are valuable implementations in lib/APINotes/* which we could use for a generic APINotes framework in Clang. For instance, APINotesManager::findAPINotes encapsulates the logic for finding the APINotes files, this seems to be totally independent from Swift.

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?

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").

A side note: ASTMutationListener 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 AddedAttributeToRecord), shouldn't we?

I wouldn't think so, since this mechanism modifies the nodes as part of their initial creation, not as a subsequent mutation?


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev


On Oct 13, 2020, at 12:11 PM, Gábor Márton <[hidden email]> wrote:

> 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?

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 hardcode the information into the source code of the analyzer itself.

> I wouldn't think so, since this mechanism modifies the nodes as part of their initial creation, not as a subsequent mutation?

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.

There seem to be three systematic places a consumer virtual could be called:
  1. 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.)
  2. After Sema is finished acting on a node, but before subsequent parsing.  I.e., on the Decl returned by e.g. `Actions.ActOnDeclarator()`.
    • That *would* require a call to the ASTMutationListener. (Ugh.)
  3. Immediately after a node is allocated via `*::Create()` in `Sema::ActOn*` etc.  
    • I.e., these calls would be a bit before the present placement of most `ProcessAPINotes(D)` calls.
#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.

All this could be contained in a simple override of a virtual e.g. `VisitCreatedFooDecl(FooDecl *D, bool Parsed1Instantiated0)` exposed in ASTConsumer or ExternalSemaSource.  

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.

If all the premises hold, this would be a broad gain in functionality via a familiar RecursiveASTVisitor-like interface.



On Mon, Oct 12, 2020 at 9:30 PM James Y Knight <[hidden email]> wrote:
On Mon, Oct 12, 2020 at 12:46 PM Gábor Márton <[hidden email]> wrote:
+1 for separating the Swift specific Sema logic the way you described.
However, there are valuable implementations in lib/APINotes/* which we could use for a generic APINotes framework in Clang. For instance, APINotesManager::findAPINotes encapsulates the logic for finding the APINotes files, this seems to be totally independent from Swift.

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?

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").

A side note: ASTMutationListener 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 AddedAttributeToRecord), shouldn't we?

I wouldn't think so, since this mechanism modifies the nodes as part of their initial creation, not as a subsequent mutation?



_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
In reply to this post by Manas via cfe-dev


On Mon, Oct 12, 2020 at 8:50 AM James Y Knight <[hidden email]> wrote:


On Fri, Oct 9, 2020 at 10:21 AM David Rector <[hidden email]> wrote:
IIUC APINotes wants to add attributes during parsing so that the those attributes are then treated just like real parsed attributes would be by Sema.  

Ideally this sort of thing should be able to be contained in an ASTConsumer, but it presently cannot: the only real option ASTConsumer provides is to traverse the AST post-parsing in an override of `HandleTranslationUnit()`.  This leaves APINotes no choice but to add its details directly to Sema and Parser, with the result that Swift-specific details would necessarily show up in the guts of mainline clang were this to be upstreamed.  

If ASTConsumer instead provided a comprehensive virtual interface (but with negligible overhead, somehow) to perform custom consumer actions at strategic points during parsing/Sema (e.g., call consumer virtuals in each Sema::ActOn*), this conflict probably could be, or at least could have been, avoided.

Note too that this issue resonates with one a user faced just the other day: he wanted to call `Sema::LookupName` within an ASTMatchers callback (during HandleTranslationUnit, as usual) to determine if a certain node could be rewritten w/o changing its semantics — but of course the necessary Sema/Parser scope info is long gone by that point.  If only he had the option to perform his logic on the node immediately after it was parsed, there would be no issue. Because he cannot, his only option was to laboriously recreate that state data in order to call LookupName.

In sum, the dearth of ASTConsumer virtuals hinder encapsulation, causing 1) Sema and Parser to be polluted with what should be a consumer’s details, and 2) consumers to be polluted with what should be Sema & Parser details, and making it difficult to upstream useful tools.

This is an excellent point!

It looks like the primary change in Clang which is required for APINotes is to place "ProcessAPINotes" calls into each of the Sema::ActOn* functions which creates a (subclass of) Decl. This needs to occur after creating the Decl object and processing the attributes in the source, but prior to actually acting on it (e.g. see apple's SemaDeclObjC.cpp). This call could instead be a call to a generic hook for modifying the decl, rather than specifically for processing API notes. We already even have a container for such hooks in Sema, "ExternalSemaSource", which seems like a reasonable place to add a virtual method which may modify newly-created Decls.

If such a hook mechanism is added, it then seems entirely plausible that SemaAPINotes.cpp and lib/APINotes/* can be moved from the clang-fork into swift itself without requiring significant modifications of the code, or any change in functionality of the feature.

Well, the APINotes also provide nullability information for ObjC interfaces, which is not part of Swift, but rather part of ObjC, so that still needs to be in clang.  That is, the functionality is useful for ObjC, and that doesn't make sense to be in Swift.
 
If this works out how it seems like it could, this seems like it may even be a better outcome for all parties (both the potential static analysis tooling use-cases, and for Swift).


--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev

On Wed, Oct 14, 2020 at 12:19 PM Saleem Abdulrasool <[hidden email]> wrote:


On Mon, Oct 12, 2020 at 8:50 AM James Y Knight <[hidden email]> wrote:


On Fri, Oct 9, 2020 at 10:21 AM David Rector <[hidden email]> wrote:
IIUC APINotes wants to add attributes during parsing so that the those attributes are then treated just like real parsed attributes would be by Sema.  

Ideally this sort of thing should be able to be contained in an ASTConsumer, but it presently cannot: the only real option ASTConsumer provides is to traverse the AST post-parsing in an override of `HandleTranslationUnit()`.  This leaves APINotes no choice but to add its details directly to Sema and Parser, with the result that Swift-specific details would necessarily show up in the guts of mainline clang were this to be upstreamed.  

If ASTConsumer instead provided a comprehensive virtual interface (but with negligible overhead, somehow) to perform custom consumer actions at strategic points during parsing/Sema (e.g., call consumer virtuals in each Sema::ActOn*), this conflict probably could be, or at least could have been, avoided.

Note too that this issue resonates with one a user faced just the other day: he wanted to call `Sema::LookupName` within an ASTMatchers callback (during HandleTranslationUnit, as usual) to determine if a certain node could be rewritten w/o changing its semantics — but of course the necessary Sema/Parser scope info is long gone by that point.  If only he had the option to perform his logic on the node immediately after it was parsed, there would be no issue. Because he cannot, his only option was to laboriously recreate that state data in order to call LookupName.

In sum, the dearth of ASTConsumer virtuals hinder encapsulation, causing 1) Sema and Parser to be polluted with what should be a consumer’s details, and 2) consumers to be polluted with what should be Sema & Parser details, and making it difficult to upstream useful tools.

This is an excellent point!

It looks like the primary change in Clang which is required for APINotes is to place "ProcessAPINotes" calls into each of the Sema::ActOn* functions which creates a (subclass of) Decl. This needs to occur after creating the Decl object and processing the attributes in the source, but prior to actually acting on it (e.g. see apple's SemaDeclObjC.cpp). This call could instead be a call to a generic hook for modifying the decl, rather than specifically for processing API notes. We already even have a container for such hooks in Sema, "ExternalSemaSource", which seems like a reasonable place to add a virtual method which may modify newly-created Decls.

If such a hook mechanism is added, it then seems entirely plausible that SemaAPINotes.cpp and lib/APINotes/* can be moved from the clang-fork into swift itself without requiring significant modifications of the code, or any change in functionality of the feature.

Well, the APINotes also provide nullability information for ObjC interfaces, which is not part of Swift, but rather part of ObjC, so that still needs to be in clang.  That is, the functionality is useful for ObjC, and that doesn't make sense to be in Swift.

Theoretically one could imagine doing so -- but TTBOMK, it's never actually been used like this. All nullability annotations used by objc compilations live in the headers themselves, and the apinotes command-line options are passed only by Swift, for creating a Swift-ObjC-bridge module.


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev


On Oct 14, 2020, at 6:49 PM, James Y Knight via cfe-dev <[hidden email]> wrote:


If such a hook mechanism is added, it then seems entirely plausible that SemaAPINotes.cpp and lib/APINotes/* can be moved from the clang-fork into swift itself without requiring significant modifications of the code, or any change in functionality of the feature.

Well, the APINotes also provide nullability information for ObjC interfaces, which is not part of Swift, but rather part of ObjC, so that still needs to be in clang.  That is, the functionality is useful for ObjC, and that doesn't make sense to be in Swift.

Theoretically one could imagine doing so -- but TTBOMK, it's never actually been used like this. All nullability annotations used by objc compilations live in the headers themselves, and the apinotes command-line options are passed only by Swift, for creating a Swift-ObjC-bridge module. 

Nullability isn’t just useful for swift, it is also useful to the clang static analyzer and other tools that want to reason about C API semantics.

-Chris

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
On Thu, Oct 15, 2020 at 11:59 PM Chris Lattner <[hidden email]> wrote:
On Oct 14, 2020, at 6:49 PM, James Y Knight via cfe-dev <[hidden email]> wrote:


If such a hook mechanism is added, it then seems entirely plausible that SemaAPINotes.cpp and lib/APINotes/* can be moved from the clang-fork into swift itself without requiring significant modifications of the code, or any change in functionality of the feature.

Well, the APINotes also provide nullability information for ObjC interfaces, which is not part of Swift, but rather part of ObjC, so that still needs to be in clang.  That is, the functionality is useful for ObjC, and that doesn't make sense to be in Swift.

Theoretically one could imagine doing so -- but TTBOMK, it's never actually been used like this. All nullability annotations used by objc compilations live in the headers themselves, and the apinotes command-line options are passed only by Swift, for creating a Swift-ObjC-bridge module. 

Nullability isn’t just useful for swift, it is also useful to the clang static analyzer and other tools that want to reason about C API semantics.

Nullability is being used in ObjC to emit warnings, but apinotes is not. For ObjC compiles (and static analyzer), the nullability information comes from header files, _Nullable and _Nonnull keywords, "NS_ASSUME_NONNULL_BEGIN", etc.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev


On Fri, Oct 16, 2020 at 4:28 PM James Y Knight via cfe-dev <[hidden email]> wrote:
On Thu, Oct 15, 2020 at 11:59 PM Chris Lattner <[hidden email]> wrote:
On Oct 14, 2020, at 6:49 PM, James Y Knight via cfe-dev <[hidden email]> wrote:


If such a hook mechanism is added, it then seems entirely plausible that SemaAPINotes.cpp and lib/APINotes/* can be moved from the clang-fork into swift itself without requiring significant modifications of the code, or any change in functionality of the feature.

Well, the APINotes also provide nullability information for ObjC interfaces, which is not part of Swift, but rather part of ObjC, so that still needs to be in clang.  That is, the functionality is useful for ObjC, and that doesn't make sense to be in Swift.

Theoretically one could imagine doing so -- but TTBOMK, it's never actually been used like this. All nullability annotations used by objc compilations live in the headers themselves, and the apinotes command-line options are passed only by Swift, for creating a Swift-ObjC-bridge module. 

Nullability isn’t just useful for swift, it is also useful to the clang static analyzer and other tools that want to reason about C API semantics.

Nullability is being used in ObjC to emit warnings, but apinotes is not. For ObjC compiles (and static analyzer), the nullability information comes from header files, _Nullable and _Nonnull keywords, "NS_ASSUME_NONNULL_BEGIN", etc.

It could be useful, however, to annotate a parameter of a C function as "nonnull" with APINotes. Take for example the GNU libc implementation for the POSIX 'getsockname' function, which does not have the __nonnull attribute written in the declaration, even though semantically the 2nd and 3rd arguments should never be NULL. 
  extern int getsockname (int __fd, __SOCKADDR_ARG __addr, socklen_t *__restrict __len) __THROW;
This is another use-case for the static analyzer, if we could describe this in an additional YAML file. Strictly speaking, we can provide this information hardcoded in the static analyzer code (as we do now), but this feature could be useful for other libraries. I mean either we can provide the annotations in the headers themselves or in additional APINotes or we hardcode them in the static analyzer code.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Adding Support for APINotes

Manas via cfe-dev
In reply to this post by Manas via cfe-dev


On Oct 16, 2020, at 7:27 AM, James Y Knight <[hidden email]> wrote:

On Thu, Oct 15, 2020 at 11:59 PM Chris Lattner <[hidden email]> wrote:
On Oct 14, 2020, at 6:49 PM, James Y Knight via cfe-dev <[hidden email]> wrote:


If such a hook mechanism is added, it then seems entirely plausible that SemaAPINotes.cpp and lib/APINotes/* can be moved from the clang-fork into swift itself without requiring significant modifications of the code, or any change in functionality of the feature.

Well, the APINotes also provide nullability information for ObjC interfaces, which is not part of Swift, but rather part of ObjC, so that still needs to be in clang.  That is, the functionality is useful for ObjC, and that doesn't make sense to be in Swift.

Theoretically one could imagine doing so -- but TTBOMK, it's never actually been used like this. All nullability annotations used by objc compilations live in the headers themselves, and the apinotes command-line options are passed only by Swift, for creating a Swift-ObjC-bridge module. 

Nullability isn’t just useful for swift, it is also useful to the clang static analyzer and other tools that want to reason about C API semantics.

Nullability is being used in ObjC to emit warnings, but apinotes is not. For ObjC compiles (and static analyzer), the nullability information comes from header files, _Nullable and _Nonnull keywords, "NS_ASSUME_NONNULL_BEGIN", etc.

Right, but APINotes are complementary to header annotations.  Some headers are readonly, e.g. if they are uncooperative system linux headers that won’t adopt the annotations.

-Chris

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev