Re: Implict casts disappeared from syntactic init list expressions in C++
Ping...
Il 29/08/2015 10:01, Abramo Bagnara ha scritto:
> Il 28/08/2015 23:27, Richard Smith ha scritto:
>> On Tue, Aug 25, 2015 at 10:27 AM, Abramo Bagnara
>> mailto:abramo.bagn...@bugseng.com>> wrote:
>>
>> Comparing the result of InitListExpr::getSyntacticForm between r224986
>> and r245836 I've discovered that integer to char implicit cast for
>> integer literal 3 is no longer added to AST for C++ (while it is present
>> in C).
>>
>> This is the source used to test:
>>
>> char v[10] = { 3 };
>>
>> Taken in account that:
>>
>> - implicit cast (and other conversions, constructor calls, etc.) are
>> very important also for who need to visit the syntactic form (obvious in
>> *both* C and C++)
>>
>> - to generate that for the syntactic form permit to increase the
>> efficiency and the sharing when using designated range extensions (as
>> the conversion chain don't need to be replicated for each entry)
>>
>> I think it is a regression. Am I missing something?
>>
>>
>> Why do you expect this semantic information to appear in the syntactic
>> form of the initializer?
>
> Compare:
>
> int x = 2.0;
>
> with
>
> struct s {
> int x;
> } v = { .x = 2.0 };
>
> For first declaration I have non-syntactic nodes (namely
> ImplicitCastExpr) along with syntactic nodes, while for the second I
> don't have that (for C++). This is an obstacle to write semi-syntactic
> checkers that aims to find e.g. implicit cast from double to int in its
> syntactic context.
> Note that although we might visit the semantic form, we'll lose the
> designators (not present in semantic form).
>
> To resume, the reason why I would expect that are:
>
> 1) this is how it always has worked for C (and fortunately still works
> this way)
>
>
> 2) this is how it always has worked (although partially, there was some
> bugs) for C++. In past we have had patches to fix the areas where this
> invariant was not respected (see commit 3146766
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20111212/050339.html
> as an example).
>
> This behavior has changed rather recently (if you think it is useful I
> can find the commit that has removed the implicit casts from syntactic
> form in C++)
>
> Before such commit(s) the only bug I was aware where AST was missing
> conversion chain was the following:
>
> struct R2 {
> R2(int) {
> }
> };
> R2 v2[] = { 1.0 };
>
>
> 3) this way it would be congruent with other areas of AST where we have
> non-syntactic nodes along with syntactic ones
>
>
> 4) it would permit to share more nodes in semantic form (and avoid to
> rebuild many times the same conversion chain).
>
> Looking at following typescript you can observe that ImplicitCastExpr is
> shared only for C, but not for C++. I've initialized only two entries,
> but it might be 1000 or 1000.
>
> $ cat p.c
> int x[100] = { [0 ... 1] = 3.0 };
> $ clang-3.8 -cc1 -ast-dump -x c p.c
> TranslationUnitDecl 0x272de40 <>
> |-TypedefDecl 0x272e338 <> implicit
> __int128_t '__int128'
> |-TypedefDecl 0x272e398 <> implicit
> __uint128_t 'unsigned __int128'
> |-TypedefDecl 0x272e648 <> implicit
> __builtin_va_list 'struct __va_list_tag [1]'
> `-VarDecl 0x272e718 col:5 x 'int [100]' cinit
> `-InitListExpr 0x272e8b0 'int [100]'
> |-array filler
> | `-ImplicitValueInitExpr 0x272e918 <> 'int'
> |-ImplicitCastExpr 0x272e900 'int'
> | `-FloatingLiteral 0x272e7f8 'double' 3.00e+00
> `-ImplicitCastExpr 0x272e900 'int'
> `-FloatingLiteral 0x272e7f8 'double' 3.00e+00
> $ clang-3.8 -cc1 -ast-dump -x c++ p.c
> TranslationUnitDecl 0x3300e60 <>
> |-TypedefDecl 0x3301398 <> implicit
> __int128_t '__int128'
> |-TypedefDecl 0x33013f8 <> implicit
> __uint128_t 'unsigned __int128'
> |-TypedefDecl 0x3301718 <> implicit
> __builtin_va_list 'struct __va_list_tag [1]'
> `-VarDecl 0x33017e8 col:5 x 'int [100]' cinit
> `-InitListExpr 0x3301980 'int [100]'
> |-array filler
> | `-ImplicitValueInitExpr 0x3301a00 <> 'int'
> |-ImplicitCastExpr 0x33019d0 'int'
> | `-FloatingLiteral 0x33018c8 'double' 3.00e+00
> `-ImplicitCastExpr 0x33019e8 'int'
> `-FloatingLiteral 0x33018c8 'double' 3.00e+00
>
>
> 5) if we would visit semantic form in a checker searching for implicit
> cast from float to int in the C source above we'll find *two* of them,
> while syntactically we have only one. This means that we should be
> forced do some dirty tricks to avoid double reporting.
>
--
Abramo Bagnara
BUGSENG srl - http://bugseng.com
mailto:abramo.bagn...@bugseng.com
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D12827: [analyzer] fix an error finding clang path
honggyu.kim created this revision.
honggyu.kim added reviewers: ayartsev, zaks.anna, krememek.
honggyu.kim added subscribers: jordan_rose, dcoughlin, cfe-commits.
This patch fixes an error made by rL247466.
It mistakenly wrote "-d" check in if statement with the file name itself not a
directory.
http://reviews.llvm.org/D12827
Files:
tools/scan-build/scan-build
Index: tools/scan-build/scan-build
===
--- tools/scan-build/scan-build
+++ tools/scan-build/scan-build
@@ -1644,9 +1644,9 @@
# Find 'clang'
if (!defined $Options{AnalyzerDiscoveryMethod}) {
- $Clang = Cwd::realpath("$RealBin/bin/clang") if (-d "$RealBin/bin/clang");
+ $Clang = Cwd::realpath("$RealBin/bin/clang") if (-d "$RealBin/bin");
if (!defined $Clang || ! -x $Clang) {
-$Clang = Cwd::realpath("$RealBin/clang") if (-d "$RealBin/clang");
+$Clang = Cwd::realpath("$RealBin/clang") if (-d "$RealBin");
}
if (!defined $Clang || ! -x $Clang) {
if (!$RequestDisplayHelp && !$ForceDisplayHelp) {
Index: tools/scan-build/scan-build
===
--- tools/scan-build/scan-build
+++ tools/scan-build/scan-build
@@ -1644,9 +1644,9 @@
# Find 'clang'
if (!defined $Options{AnalyzerDiscoveryMethod}) {
- $Clang = Cwd::realpath("$RealBin/bin/clang") if (-d "$RealBin/bin/clang");
+ $Clang = Cwd::realpath("$RealBin/bin/clang") if (-d "$RealBin/bin");
if (!defined $Clang || ! -x $Clang) {
-$Clang = Cwd::realpath("$RealBin/clang") if (-d "$RealBin/clang");
+$Clang = Cwd::realpath("$RealBin/clang") if (-d "$RealBin");
}
if (!defined $Clang || ! -x $Clang) {
if (!$RequestDisplayHelp && !$ForceDisplayHelp) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12827: [analyzer] fix an error finding clang path
honggyu.kim added a comment. After applying http://reviews.llvm.org/rL247466, I got the following error which was not shown before. scan-build: error: Cannot find an executable 'clang' relative to scan-build. Consider using --use-analyzer to pick a version of 'clang' to use for static analysis. Please have a look at it. http://reviews.llvm.org/D12827 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12827: [analyzer] fix an error finding clang path
honggyu.kim updated the summary for this revision.
honggyu.kim updated this revision to Diff 34624.
http://reviews.llvm.org/D12827
Files:
tools/scan-build/scan-build
Index: tools/scan-build/scan-build
===
--- tools/scan-build/scan-build
+++ tools/scan-build/scan-build
@@ -1644,9 +1644,9 @@
# Find 'clang'
if (!defined $Options{AnalyzerDiscoveryMethod}) {
- $Clang = Cwd::realpath("$RealBin/bin/clang") if (-d "$RealBin/bin/clang");
+ $Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
if (!defined $Clang || ! -x $Clang) {
-$Clang = Cwd::realpath("$RealBin/clang") if (-d "$RealBin/clang");
+$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
}
if (!defined $Clang || ! -x $Clang) {
if (!$RequestDisplayHelp && !$ForceDisplayHelp) {
Index: tools/scan-build/scan-build
===
--- tools/scan-build/scan-build
+++ tools/scan-build/scan-build
@@ -1644,9 +1644,9 @@
# Find 'clang'
if (!defined $Options{AnalyzerDiscoveryMethod}) {
- $Clang = Cwd::realpath("$RealBin/bin/clang") if (-d "$RealBin/bin/clang");
+ $Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
if (!defined $Clang || ! -x $Clang) {
-$Clang = Cwd::realpath("$RealBin/clang") if (-d "$RealBin/clang");
+$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
}
if (!defined $Clang || ! -x $Clang) {
if (!$RequestDisplayHelp && !$ForceDisplayHelp) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: PATCH: Expose the 'file' that is associated with a compile database command
Test would be awesome :) Thx! On Fri, Sep 11, 2015 at 10:44 PM Argyrios Kyrtzidis wrote: > In r247468, thanks for reviewing! > > On Sep 11, 2015, at 10:24 AM, Manuel Klimek via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > Ok, looked at the original patch again, and if we're fixing the > FixedCompilationDatabase to only insert the file when it actually produces > a CompileCommand it seems to be fine. > > On Fri, Sep 11, 2015 at 6:32 PM Argyrios Kyrtzidis > wrote: > >> On Sep 11, 2015, at 12:21 AM, Manuel Klimek wrote: >> >> On Thu, Sep 10, 2015 at 8:36 PM Argyrios Kyrtzidis >> wrote: >> >>> On Sep 10, 2015, at 1:48 AM, Manuel Klimek wrote: >>> >>> @@ -179,11 +185,13 @@ public: >>>/// \param Directory The base directory used in the >>> FixedCompilationDatabase. >>>static FixedCompilationDatabase *loadFromCommandLine(int &Argc, >>> const char >>> *const *Argv, >>> - Twine Directory >>> = "."); >>> + Twine Directory >>> = ".", >>> + Twine File = >>> Twine()); >>> >>> A fixed compilation database returns the same command lines for all >>> files, thus having a file in the function seems strange. >>> >>> >>> Ah ok, thanks for clarifying. >>> >>> >>> What exactly is the use case? So far, the compilation database has been >>> designed for 2 use cases: >>> 1. for a file, get the compile commands; the user already knows the >>> file, no need to get the file >>> 2. get all compile commands; for that, we have the getAllFiles() method, >>> so a user can get all known files (for compilation databases that support >>> that), and then get the compile command line. >>> >>> >>> It’s #2, I want to get all compile commands. But it seems really strange >>> to me that the ‘file’ starts as a property of the compile command in the >>> json file but then it gets dropped and I need to do work to re-associate >>> the files with the compile commands again. >>> >> >> The JSON file format is one possible implementation for the >> compilation-database interface. The FixedCompilationDatabase is another >> one, that doesn't have any information on files. >> >> >>> I need to get a list of all the files and then for each one do a lookup >>> to get the associated commands. I then have to maintain this association >>> myself, passing a command along with its file separately or the structure >>> that keeps track of the association. >>> >>> It seems simpler to me to include the file that was associated with the >>> command (if the compilation database supports that) along with the command, >>> is there a downside I’m missing ? >>> >> >> Well, to me, it's a design question - if it also makes sense to have a >> CompileCommand without a file associated with it, putting the file in >> there, while convenient, is a design smell. >> >> >> It can be optional to communicate that it may not be there. Note that, >> IMO, having multiple files and compile commands for them is the >> overwhelmingly most common use of the compilation database. >> >> That said, I'm happy to be convinced that I'm wrong :) I guess I don't >> see yet that keeping track of the files outside is more than one line of >> extra code. >> >> >> Not sure what *one* line this is, I have to declare a map and then >> populate it, no ? And to do it in c-index-test it would take way more that >> one line. >> But it is also beyond populating a map, this has an effect on APIs using >> a CompileCommand. For example: >> >> void doSomethingWithCompileCommand(const CompileCommand &cmd); >> >> Ah it would be useful to know the file that this command was associated >> with: >> >> void doSomethingWithCompileCommand(const CompileCommand &cmd, StringRef >> filename); >> >> What do I have now ? This is a function taking a command and a string for >> the filename separately. >> Is this flexibility useful ? Does it make sense to pass any filename >> there ? No there’s only one filename that the command was associated with >> so this ‘flexibility’ only increases the complexity of using the function. >> And this can propagate to other function’s callees. >> This seems like a design smell to me. >> >> >> Cheers, >> /Manuel >> >> >>> >>> >>> Thoughts? >>> /Manuel >>> >>> On Wed, Sep 9, 2015 at 9:36 PM Argyrios Kyrtzidis >>> wrote: >>> Hi, The attached patch exposes the ‘file’ entry in a compilation database command, via the CompileCommand structure. >>> >>> ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: recordDecl() AST matcher
On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman wrote: > On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith > wrote: > > I don't think CXXRecordDecl is an anachronism, so much as an > implementation > > detail; it makes sense to use a smaller class when in C mode, as we don't > > need most of the features and complexity that CXXRecordDecl brings with > it. > > But... as a user of clang matchers, I don't think I'd want to care about > the > > difference, and it'd be more convenient if I could nest (say) a hasMethod > > matcher within a recordDecl matcher, since it's completely obvious what > that > > should mean. If I have a matcher that says: > > > > recordDecl(or(hasMethod(...), hasField(...))) > > > > I would expect that to work in both C and C++ (and the only way it could > > match in C would be on a record with the specified field, since the > > hasMethod matcher would always fail). > > Okay, so then it sounds like we want recordDecl to *mean* RecordDecl, > but we want the traversal and narrowing matchers that currently take a > CXXRecordDecl to instead take a RecordDecl and handle the CXX part > transparently? This means we would not need to add a cxxRecordDecl() > matcher, but could still access CXX-only functionality (like access > control, base classes, etc) through recordDecl()? > I'm against that proposal. I think we have tried to make the matchers more "user friendly" in the past, and all those attempts have failed miserably; in the end, users will do ast-dump to see what they want to match, and then be confused when the matchers do follow the AST 99% of the time, but try to be smart 1% of the time. I think given that we want to keep CXXRecordDecl, the right solution is to have a cxxRecordDecl() matcher. Richard: if CXXRecordDecl was really an implementation detail, it would be hidden behind a RecordDecl class, as an implementation detail. The reasons why we don't want it to be an implementation detail in the code (performance, data structure size) don't matter - in the end, it's in the AST API. > > ~Aaron > > > > > On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek > wrote: > >> > >> Richard! We need an informed opinion :D > >> > >> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman > >> wrote: > >>> > >>> Ping? > >>> > >>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek > wrote: > >>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman > > >>> > wrote: > >>> >> > >>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek > >>> >> wrote: > >>> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman > >>> >> > > >>> >> > wrote: > >>> >> >> > >>> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek > > >>> >> >> wrote: > >>> >> >> > Yea, we had that discussion a few times, and I can never > remember > >>> >> >> > why > >>> >> >> > we > >>> >> >> > ended up in the state we're in. > >>> >> >> > We definitely had a time where we switched to just using the > >>> >> >> > exact > >>> >> >> > same > >>> >> >> > name > >>> >> >> > as the node's class name for the matchers. > >>> >> >> > I *think* we didn't do it for cxxRecordDecl, because Richard > said > >>> >> >> > that's > >>> >> >> > a > >>> >> >> > relic we should get rid of anyway, but I'm not sure. > >>> >> >> > >>> >> >> FWIW, I think the state we're in is the worst of all worlds. It's > >>> >> >> not > >>> >> >> intuitive that recordDecl() doesn't match a struct in C mode, and > >>> >> >> as > >>> >> >> it stands, there is no way to match a struct or union declaration > >>> >> >> in C > >>> >> >> at all. > >>> >> > > >>> >> > > >>> >> > Agreed. Best intentions. Worst possible outcome. That's software > >>> >> > development > >>> >> > :) > >>> >> > > >>> >> >> > > >>> >> >> > On Fri, Sep 4, 2015 at 8:32 PM Aaron Ballman > >>> >> >> > > >>> >> >> > wrote: > >>> >> >> >> > >>> >> >> >> It turns out that the recordDecl() AST matcher doesn't match > >>> >> >> >> RecordDecl objects; instead, it matches CXXRecordDecl objects. > >>> >> >> >> This > >>> >> >> >> is... unfortunate... as it makes writing AST matchers more > >>> >> >> >> complicated > >>> >> >> >> because of having to translate between > >>> >> >> >> recordDecl()/CXXRecordDecl. > >>> >> >> >> It > >>> >> >> >> also makes it impossible to match a struct or union > declaration > >>> >> >> >> in C > >>> >> >> >> or ObjC. However, given how prevalent recordDecl()'s use is in > >>> >> >> >> the > >>> >> >> >> wild (I'm guessing), changing it at this point would be a Bad > >>> >> >> >> Thing. > >>> >> >> >> > >>> >> >> >> For people trying to write AST matchers for languages like C > or > >>> >> >> >> ObjC, > >>> >> >> >> I would like to propose adding: > >>> >> >> >> > >>> >> >> >> structDecl() > >>> >> >> >> unionDecl() > >>> >> >> >> tagDecl() > >>> >> >> >> > >>> >> >> >> These will match nicely with the existing enumDecl() AST > >>> >> >> >> matcher. > >>> >> >> >> > >>> >> >> >> Additionally, I would like to add cxxRecordDecl() to match > >>> >> >> >> CXXRecordDecl objects. While it duplicates the functiona
r247503 - Test commit.
Author: kli Date: Sat Sep 12 08:35:31 2015 New Revision: 247503 URL: http://llvm.org/viewvc/llvm-project?rev=247503&view=rev Log: Test commit. Modified: cfe/trunk/lib/Sema/SemaOpenMP.cpp Modified: cfe/trunk/lib/Sema/SemaOpenMP.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOpenMP.cpp?rev=247503&r1=247502&r2=247503&view=diff == --- cfe/trunk/lib/Sema/SemaOpenMP.cpp (original) +++ cfe/trunk/lib/Sema/SemaOpenMP.cpp Sat Sep 12 08:35:31 2015 @@ -7193,6 +7193,5 @@ OMPClause *Sema::ActOnOpenMPDeviceClause return nullptr; } } - return new (Context) OMPDeviceClause(ValExpr, StartLoc, LParenLoc, EndLoc); } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r247510 - Use -f instead of -d flag for testing existing of clang executable (http://reviews.llvm.org/D12827).
Author: kremenek
Date: Sat Sep 12 11:01:34 2015
New Revision: 247510
URL: http://llvm.org/viewvc/llvm-project?rev=247510&view=rev
Log:
Use -f instead of -d flag for testing existing of clang executable
(http://reviews.llvm.org/D12827).
Modified:
cfe/trunk/tools/scan-build/scan-build
Modified: cfe/trunk/tools/scan-build/scan-build
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/scan-build/scan-build?rev=247510&r1=247509&r2=247510&view=diff
==
--- cfe/trunk/tools/scan-build/scan-build (original)
+++ cfe/trunk/tools/scan-build/scan-build Sat Sep 12 11:01:34 2015
@@ -1644,9 +1644,9 @@ if (!@ARGV and !$RequestDisplayHelp) {
# Find 'clang'
if (!defined $Options{AnalyzerDiscoveryMethod}) {
- $Clang = Cwd::realpath("$RealBin/bin/clang") if (-d "$RealBin/bin/clang");
+ $Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
if (!defined $Clang || ! -x $Clang) {
-$Clang = Cwd::realpath("$RealBin/clang") if (-d "$RealBin/clang");
+$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
}
if (!defined $Clang || ! -x $Clang) {
if (!$RequestDisplayHelp && !$ForceDisplayHelp) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12827: [analyzer] fix an error finding clang path
krememek accepted this revision. krememek added a comment. This revision is now accepted and ready to land. Applied in r247510. http://reviews.llvm.org/D12827 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12402: PR24595: clang-cl fails to compile vswriter.h header from Windows SDK 8.1 in 32 bit mode
andreybokhanko added inline comments.
Comment at: lib/Sema/SemaType.cpp:5876
@@ +5875,3 @@
+
+if (!IsCtorOrDtor) {
+ if (CurCC != DefaultCC || DefaultCC == ToCC)
rnk wrote:
> This looks like the !IsCtorOrDtor check that affects Itanium. Isn't it
> already handled for MS C++ above?
Reid, now I'm completely confused... I thought you asked me to insert this
check for Itanium ABI as well (in your comment from Sep 1st). Am I mistaken? If
yes, I can remove it and restore ctor-dtor-alias.cpp test back to original
version.
http://reviews.llvm.org/D12402
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12402: PR24595: clang-cl fails to compile vswriter.h header from Windows SDK 8.1 in 32 bit mode
rnk added inline comments.
Comment at: lib/Sema/SemaType.cpp:5876
@@ +5875,3 @@
+
+if (!IsCtorOrDtor) {
+ if (CurCC != DefaultCC || DefaultCC == ToCC)
andreybokhanko wrote:
> rnk wrote:
> > This looks like the !IsCtorOrDtor check that affects Itanium. Isn't it
> > already handled for MS C++ above?
> Reid, now I'm completely confused... I thought you asked me to insert this
> check for Itanium ABI as well (in your comment from Sep 1st). Am I mistaken?
> If yes, I can remove it and restore ctor-dtor-alias.cpp test back to original
> version.
Sorry, it shouldn't affect Itanium. What you have without this check is fine.
http://reviews.llvm.org/D12402
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.
zaks.anna accepted this revision.
zaks.anna added a comment.
This revision is now accepted and ready to land.
Please, commit after the comments are addressed!
Thank you,
Anna.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:39
@@ -26,1 +38,3 @@
+// ProgramState trait - a map from symbol to its specialized type.
+REGISTER_MAP_WITH_PROGRAMSTATE(TypeParamMap, SymbolRef,
I think this needs more explanation. We already have another map from a symbol
to it's type. What is different about this one?
Maybe add something along these lines:
The type inflation is tracked by DynamicTypeMap.This is an auxiliary map that
tracks more information in some cases where the known specialized type is an
ancestor of the most precise type.
Maybe the name of the map could be enhanced as well? Now you have TypeParamMap
and DynamicTypeMap..
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:121
@@ -54,3 +120,3 @@
I != E; ++I) {
if (!SR.isLiveRegion(I->first)) {
State = State->remove(I->first);
It's odd that we are using this API.. Are we keeping track of non-symbolic
regions? If not, can't we just check if Region->getSymbol() is dead?
(Same in the nullability checker.)
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:275
@@ +274,3 @@
+///
+/// Precondition: the cast is between ObjCObjectPointers.
+ExplodedNode *DynamicTypePropagation::dynamicTypePropagationOnCasts(
I do not see where you are checking the precondition.
http://reviews.llvm.org/D12381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Bug 23529: Add support for gcc's attribute abi_tag (needed for compatibility with gcc 5's libstdc++)
Hi all,
I've been working on #23529.
The abi tag mangling implemented by gcc is horrible, but I think my
patch covers most of the incompatibilities with gcc5.
There might be some bugs with substitutions, although I have to come up
with a test case for that to see what gcc does...
Test cases comparing gcc and patched clang++:
http://files.stbuehler.de/test-itanium-mangle.html
(generated with http://files.stbuehler.de/test-itanium-mangle.sh, also
attached)
regards,
Stefan
Author: Stefan Bühler
add gcc abi_tag support
- parse abi_tag attribute
- emit abi tags in name Itanium Mangling
- try to emit the same tags as gcc5
diff --git a/include/clang/Basic/Attr.td b/include/clang/Basic/Attr.td
index 4b8a7b7..8f8b9ba 100644
--- a/include/clang/Basic/Attr.td
+++ b/include/clang/Basic/Attr.td
@@ -344,6 +344,14 @@ class IgnoredAttr : Attr {
// Attributes begin here
//
+def AbiTag : Attr {
+ let Spellings = [GCC<"abi_tag">];
+ let Args = [VariadicStringArgument<"Tags">];
+ let Subjects = SubjectList<[Struct, Var, Function, Namespace], ErrorDiag,
+ "ExpectedStructClassVariableFunctionMethodOrInlineNamespace">;
+ let Documentation = [Undocumented];
+}
+
def AddressSpace : TypeAttr {
let Spellings = [GNU<"address_space">];
let Args = [IntArgument<"AddressSpace">];
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 6ac5748..17f46fa 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2336,7 +2336,8 @@ def warn_attribute_wrong_decl_type : Warning<
"Objective-C instance methods|init methods of interface or class extension declarations|"
"variables, functions and classes|Objective-C protocols|"
"functions and global variables|structs, unions, and typedefs|structs and typedefs|"
- "interface or protocol declarations|kernel functions}1">,
+ "interface or protocol declarations|kernel functions|"
+ "structs, classes, variables, functions, methods and inline namespaces}1">,
InGroup;
def err_attribute_wrong_decl_type : Error;
def warn_type_attribute_wrong_type : Warning<
@@ -4013,6 +4014,13 @@ def err_definition_of_explicitly_defaulted_member : Error<
def err_redefinition_extern_inline : Error<
"redefinition of a 'extern inline' function %0 is not supported in "
"%select{C99 mode|C++}1">;
+def err_attr_abi_tag_only_on_inline_namespace :
+ Error<"abi_tag attribute only allowed on inline namespaces">;
+def err_abi_tag_on_redeclaration :
+ Error<"cannot add abi_tag attribute in redeclaration">;
+def err_new_abi_tag_on_redeclaration :
+ Error<"abi_tag %0 missing in original declaration">;
+
def note_deleted_dtor_no_operator_delete : Note<
"virtual destructor requires an unambiguous, accessible 'operator delete'">;
diff --git a/include/clang/Sema/AttributeList.h b/include/clang/Sema/AttributeList.h
index ca456b2..c3f0841 100644
--- a/include/clang/Sema/AttributeList.h
+++ b/include/clang/Sema/AttributeList.h
@@ -852,7 +852,8 @@ enum AttributeDeclKind {
ExpectedStructOrUnionOrTypedef,
ExpectedStructOrTypedef,
ExpectedObjectiveCInterfaceOrProtocol,
- ExpectedKernelFunction
+ ExpectedKernelFunction,
+ ExpectedStructClassVariableFunctionMethodOrInlineNamespace
};
} // end namespace clang
diff --git a/lib/AST/ItaniumMangle.cpp b/lib/AST/ItaniumMangle.cpp
index 3f40743..bb16ebe 100644
--- a/lib/AST/ItaniumMangle.cpp
+++ b/lib/AST/ItaniumMangle.cpp
@@ -214,6 +214,8 @@ public:
class CXXNameMangler {
ItaniumMangleContextImpl &Context;
raw_ostream &Out;
+ bool NullOut = false;
+ bool DisableDerivedAbiTags = false;
/// The "structor" is the top-level declaration being mangled, if
/// that's not a template specialization; otherwise it's the pattern
@@ -263,6 +265,167 @@ class CXXNameMangler {
} FunctionTypeDepth;
+ // abi_tag is a gcc attribute, taking one or more strings called "tags".
+ //
+ // the goal is to annotage against which version of a library an object was
+ // build and to be able to provide backwards compatibility ("dual abi").
+ //
+ // for this the emitted mangled names have to be different, while you don't
+ // want the user to have to use different names in the source.
+ //
+ // the abi_tag can be present on Struct, Var and Function declarations as
+ // "explicit" tag, and on inline Namespace as "implicit" tag. Explicit tags
+ // are always emitted after the unqualified name, and (implicit) tags on
+ // namespace are not.
+ //
+ // For functions and variables there is a set of "implicitly available"
+ // tags. These tags are: all tags from the namespace/structs the name is
+ // embedded in, all tags from any template arguments of the name, and, for
+ // functions, alls tags used anywhere in the (i.e.
+ // parameters and sometimes the return type).
+ //
+ // For functions this is basically the list of all tags from the signature
+ // without the unqualifi
Re: [PATCH] D12761: MPI-Checker patch for Clang Static Analyzer
Alexander_Droste added inline comments.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:10-11
@@ +9,4 @@
+///
+/// \file
+/// This file defines convenience templates for C++ container class usage.
+///
gribozavr wrote:
> This file re-invents a lot of APIs that are currently under review by the C++
> committee under the Ranges effort. I think most of the wrappers are more or
> less trivial and should use STL directly, or the wrappers should match
> exactly the currently proposed STL APIs.
Ok, I'll try to mimic the API of the proposal. Can I keep the `cont::`
namespace?
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:88-109
@@ +87,24 @@
+
+/// \brief Deletes element at given index.
+///
+/// \param container
+/// \param index
+template void eraseIndex(T &container, size_t idx) {
+ container.erase(container.begin() + idx);
+}
+
+/// \brief Sort with default criterion.
+///
+/// \param container
+template void sort(T &container) {
+ std::sort(container.begin(), container.end());
+}
+
+/// \brief Sort by given predicate.
+///
+/// \param container
+/// \param predicate
+template void sortPred(T &container, P predicate) {
+ std::sort(container.begin(), container.end(), predicate);
+}
+
gribozavr wrote:
> I question the value of such trivial wrappers.
I like to use them because they make things a little bit more compact.
`sort(container.begin(), container.end(), predicate)`
vs.
`sortPred(container, predicate)`
http://reviews.llvm.org/D12761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12761: MPI-Checker patch for Clang Static Analyzer
Alexander_Droste added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:524
@@ +523,3 @@
+def MPIChecker : Checker<"MPI-Checker">,
+ HelpText<"Checks MPI code written in C">,
+ DescFile<"MPIChecker.cpp">;
gribozavr wrote:
> Does it only works with C code, or does it work with C++ code that uses C
> bindings for MPI? I think it is safe to describe it as "Checks MPI code".
I guess you're right, it should work with C++ code that uses C bindings for MPI
but I've only evaluated and tested the checker on plain C code.
Should we delay that decision to the end of the review?
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerAST.cpp:22
@@ +21,3 @@
+
+void MPICheckerAST::initMPITypeContainer() {
+ mpiTypes_ = {"MPI_BYTE",
gribozavr wrote:
> MPICH has its header files annotated for Clang for a few years already, I
> think we should be using the annotations instead of textually matching the
> constants. Moreover, some users are using those annotations for their own
> datatypes.
I agree that using annotations is preferable, as the type matching is checked
during normal compilation but as you mentioned,
it necessary that those are implemented in a specific MPI implementation.
The check would support type matching for any implementation.
Custom buffer types and custom MPI types are simply skipped why that should not
impose a problem. Further, I planned to support custom types in the future.
They could get
passed as something like a list of pairs to the checker
{{"customBufType", "customMPI"},...}.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerAST.hpp:101
@@ +100,3 @@
+ MPIFunctionClassifier funcClassifier_;
+ llvm::SmallVector mpiTypes_;
+ MPIBugReporter bugReporter_;
gribozavr wrote:
> This should be a hashtable-based set, like `llvm::DenseSet`. Scanning this
> vector over and over again is wasteful.
`llvm::DenseSet` does not compile.
`error: no member named 'getEmptyKey' ..`
Is there an alternative?
http://reviews.llvm.org/D12761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12358: [Analyzer] Handling constant bound loops
dcoughlin added a comment. I looked at the behavior of invalidateRegions() under the patch. It looks like MemSpaceRegions //are// being added to the worklist but these regions don't have clusters associated with them so invalidating the regions themselves doesn't remove any bindings. Ted: What would you think about extending the logic in GenerateClusters() that invalidates globals for function calls to handle this more general case? That logic already checks to see if a region is in a particular global MemSpaceRegion and -- if so -- adds it to the worklist. We could, for example, add a new RegionAndSymbolInvalidationTraits (e.g., TK_InvalidateEntireMemSpace). Clients of invalidateRegions could set this trait on the MemSpace they want to invalidate and GenerateClusters() could add any region with a memory space that has this trait to the work list. Does this seem like a reasonable approach? http://reviews.llvm.org/D12358 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: recordDecl() AST matcher
On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek wrote: > > > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman > wrote: >> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith >> wrote: >> > I don't think CXXRecordDecl is an anachronism, so much as an >> > implementation >> > detail; it makes sense to use a smaller class when in C mode, as we >> > don't >> > need most of the features and complexity that CXXRecordDecl brings with >> > it. >> > But... as a user of clang matchers, I don't think I'd want to care about >> > the >> > difference, and it'd be more convenient if I could nest (say) a >> > hasMethod >> > matcher within a recordDecl matcher, since it's completely obvious what >> > that >> > should mean. If I have a matcher that says: >> > >> > recordDecl(or(hasMethod(...), hasField(...))) >> > >> > I would expect that to work in both C and C++ (and the only way it could >> > match in C would be on a record with the specified field, since the >> > hasMethod matcher would always fail). >> >> Okay, so then it sounds like we want recordDecl to *mean* RecordDecl, >> but we want the traversal and narrowing matchers that currently take a >> CXXRecordDecl to instead take a RecordDecl and handle the CXX part >> transparently? This means we would not need to add a cxxRecordDecl() >> matcher, but could still access CXX-only functionality (like access >> control, base classes, etc) through recordDecl()? > > > I'm against that proposal. I think we have tried to make the matchers more > "user friendly" in the past, and all those attempts have failed miserably; > in the end, users will do ast-dump to see what they want to match, and then > be confused when the matchers do follow the AST 99% of the time, but try to > be smart 1% of the time. > I think given that we want to keep CXXRecordDecl, the right solution is to > have a cxxRecordDecl() matcher. Personally, I think this makes the most sense, at least to me. The recommendation I've always heard (and given) is to use -ast-dump and write matchers from there. (Consequently, the more I work with type traversal matchers, the more I wish we had -ast-dump-types to give even *more* information for writing matchers.) But the question still remains, what do we do with recordDecl? Right now, it means CXXRecordDecl instead of RecordDecl. If we change it to mean RecordDecl instead, there's a chance we'll break existing, reasonable code. Are we okay with that risk? If we're at least conceptually okay with it, I could make the change locally and see just how much of our own code breaks, and report back. But if that turns out to be problematic, do we want to deprecate recordDecl and replace it with structDecl as our fallback position? Or is there a better solution? Basically, I see a few ways to solve this (and there may be other ways I'm not thinking about yet): 1) Undocument/deprecate recordDecl, add structDecl, unionDecl, and cxxRecordDecl. This does not match the AST because we have no StructDecl or UnionDecl types. The more I think about this option, the less I like it. It's easy to implement, but seems hard to relate to the AST. 2) Make recordDecl match RecordDecl, don't touch other matchers. Add way to distinguish unions from structs (e.g., isUnion(), isStruct()), add cxxRecordDecl. This matches the AST most closely, but may break code. I think that I prefer this approach, but it depends heavily on what "may break code" looks like in practice. 3) Make recordDecl match RecordDecl, fix other matchers that currently take CXXRecordDecl to instead take RecordDecl and handle sensibly when possible. Add a way to distinguish unions from structs, add cxxRecordDecl. This doesn't match the AST because we will have matchers taking a RecordDecl when the AST would require a CXXRecordDecl, but is likely to break less code. ~Aaron > Richard: if CXXRecordDecl was really an implementation detail, it would be > hidden behind a RecordDecl class, as an implementation detail. The reasons > why we don't want it to be an implementation detail in the code > (performance, data structure size) don't matter - in the end, it's in the > AST API. > >> >> >> ~Aaron >> >> > >> > On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek >> > wrote: >> >> >> >> Richard! We need an informed opinion :D >> >> >> >> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman >> >> wrote: >> >>> >> >>> Ping? >> >>> >> >>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek >> >>> wrote: >> >>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman >> >>> > >> >>> > wrote: >> >>> >> >> >>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek >> >>> >> wrote: >> >>> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman >> >>> >> > >> >>> >> > wrote: >> >>> >> >> >> >>> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek >> >>> >> >> >> >>> >> >> wrote: >> >>> >> >> > Yea, we had that discussion a few times, and I can never >> >>> >> >> > remember >> >>> >> >> > why >> >>> >> >> > we >> >>> >> >> > ended up in the state we're in. >> >>> >> >> > We
Re: Bug 23529: Add support for gcc's attribute abi_tag (needed for compatibility with gcc 5's libstdc++)
Would you mind sticking abi-tag.patch in phabricator: http://llvm.org/docs/Phabricator.html ? Your patch is big enough that it would make it a little easier for me to review. On Sat, Sep 12, 2015 at 7:12 AM, Stefan Bühler via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Hi all, > > I've been working on #23529. > > The abi tag mangling implemented by gcc is horrible, but I think my > patch covers most of the incompatibilities with gcc5. > > There might be some bugs with substitutions, although I have to come up > with a test case for that to see what gcc does... > > Test cases comparing gcc and patched clang++: > http://files.stbuehler.de/test-itanium-mangle.html > (generated with http://files.stbuehler.de/test-itanium-mangle.sh, also > attached) > > regards, > Stefan > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12827: [analyzer] fix an error finding clang path
ayartsev added a comment. Thanks, Honggyu! http://reviews.llvm.org/D12827 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12827: [analyzer] fix an error finding clang path
honggyu.kim added a comment. In http://reviews.llvm.org/D12827#244906, @ayartsev wrote: > Thanks, Honggyu! You're welcome, Антон! http://reviews.llvm.org/D12827 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12761: MPI-Checker patch for Clang Static Analyzer
Alexander_Droste updated this revision to Diff 34633.
Alexander_Droste added a comment.
- changed header file extension to .h
- fixed variable naming
- adapted llvm include guard style
- fixed function comments
- included parameter names in function declarations
- fixed namespacing (mpi is now inside of clang)
- changed initialization of member vars with nullptr from mem{nullptr} to mem =
nullptr
- reduced convenience template header for containers to single function ->
(removed unit tests)
- renamed isContained() template function for containers to contains()
http://reviews.llvm.org/D12761
Files:
tools/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerAST.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerAST.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/TranslationUnitVisitor.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/TranslationUnitVisitor.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/TypeVisitor.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.h
tools/clang/test/Analysis/MPIChecker.c
tools/clang/unittests/StaticAnalyzer/CMakeLists.txt
tools/clang/unittests/StaticAnalyzer/MPI-Checker/CMakeLists.txt
tools/clang/unittests/StaticAnalyzer/MPI-Checker/ContainerTest.cpp
tools/clang/unittests/StaticAnalyzer/MPI-Checker/UtilityTest.cpp
Index: tools/clang/unittests/StaticAnalyzer/MPI-Checker/UtilityTest.cpp
===
--- tools/clang/unittests/StaticAnalyzer/MPI-Checker/UtilityTest.cpp
+++ tools/clang/unittests/StaticAnalyzer/MPI-Checker/UtilityTest.cpp
@@ -0,0 +1,12 @@
+#include "gtest/gtest.h"
+#include "Utility.h"
+
+TEST(Utility, split) {
+ auto s = util::split("aaa:bbb", ':');
+ EXPECT_EQ(s[0], "aaa");
+ EXPECT_EQ(s[1], "bbb");
+
+ auto s2 = util::split("aaa,bbb", ',');
+ EXPECT_EQ(s2[0], "aaa");
+ EXPECT_EQ(s2[1], "bbb");
+}
Index: tools/clang/unittests/StaticAnalyzer/MPI-Checker/ContainerTest.cpp
===
--- tools/clang/unittests/StaticAnalyzer/MPI-Checker/ContainerTest.cpp
+++ tools/clang/unittests/StaticAnalyzer/MPI-Checker/ContainerTest.cpp
@@ -0,0 +1,10 @@
+#include "gtest/gtest.h"
+#include
+#include "Container.h"
+
+TEST(Container, contains) {
+ std::vector v{0, 1, 2, 3, 4, 5, 6, 8, 9};
+ EXPECT_TRUE(cont::contains(v, 0));
+ EXPECT_TRUE(cont::contains(v, 3));
+ EXPECT_TRUE(cont::contains(v, 9));
+}
Index: tools/clang/unittests/StaticAnalyzer/MPI-Checker/CMakeLists.txt
===
--- tools/clang/unittests/StaticAnalyzer/MPI-Checker/CMakeLists.txt
+++ tools/clang/unittests/StaticAnalyzer/MPI-Checker/CMakeLists.txt
@@ -0,0 +1,18 @@
+set(LLVM_LINK_COMPONENTS Support)
+
+include_directories(
+../../../lib/StaticAnalyzer/Checkers/MPI-Checker
+)
+
+add_clang_unittest(MPI-Checker
+UtilityTest.cpp
+ContainerTest.cpp
+)
+
+target_link_libraries(MPI-Checker
+ clangAST
+ clangBasic
+ clangLex
+ clangParse
+ clangSema
+ clangStaticAnalyzerCheckers)
Index: tools/clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- tools/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ tools/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -6,8 +6,10 @@
AnalyzerOptionsTest.cpp
)
+add_subdirectory(MPI-Checker)
+
target_link_libraries(StaticAnalysisTests
clangBasic
clangAnalysis
- clangStaticAnalyzerCore
+ clangStaticAnalyzerCore
)
Index: tools/clang/test/Analysis/MPIChecker.c
===
--- tools/clang/test/Analysis/MPIChecker.c
+++ tools/clang/test/Analysis/MPIChecker.c
@@ -0,0 +1,456 @@
+// RUN: %clang_cc1 -I/usr/include/ -I/usr/local/include/ -analyze -analyzer-checker=mpi.MPI-Checker -verify %s
+
+#include
+#include
+#include
+
+void doubleWait() {
+ int rank = 0;
+ double buf = 0;
+ MPI_Comm_rank(MPI_COMM_WORLD, &rank);
+
+ if (rank > 0) {
+MPI_Request req[2];
+
+MPI_Isend(&buf, 1, MPI_DOUBLE, rank + 1, 0, MPI_COMM_WORLD, &req[0]);
+MPI_Irecv(&
Re: [PATCH] D12761: MPI-Checker patch for Clang Static Analyzer
Alexander_Droste marked 14 inline comments as done.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:10-11
@@ +9,4 @@
+///
+/// \file
+/// This file defines convenience templates for C++ container class usage.
+///
Alexander_Droste wrote:
> gribozavr wrote:
> > This file re-invents a lot of APIs that are currently under review by the
> > C++ committee under the Ranges effort. I think most of the wrappers are
> > more or less trivial and should use STL directly, or the wrappers should
> > match exactly the currently proposed STL APIs.
> Ok, I'll try to mimic the API of the proposal. Can I keep the `cont::`
> namespace?
I reduced the header to a single function. Is that acceptable?
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:67
@@ +66,3 @@
+
+/// \brief Deletes first appearance of given pointer.
+///
gribozavr wrote:
> Why is this a separate overload?
I removed the function.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.hpp:22
@@ +21,3 @@
+
+namespace mpi {
+
gribozavr wrote:
> The namespace should be nested under `clang`, we shouldn't be claiming the
> top-level `mpi` namespace.
Should the `util` and `cont` namespaces be nested under `clang` too?
http://reviews.llvm.org/D12761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12761: MPI-Checker patch for Clang Static Analyzer
Alexander_Droste updated this revision to Diff 34640.
Alexander_Droste added a comment.
- fixed include guard for MPIBugReporter.h
- capitalized two variable names
http://reviews.llvm.org/D12761
Files:
tools/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerAST.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerAST.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/TranslationUnitVisitor.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/TranslationUnitVisitor.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/TypeVisitor.h
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.h
tools/clang/test/Analysis/MPIChecker.c
tools/clang/unittests/StaticAnalyzer/CMakeLists.txt
tools/clang/unittests/StaticAnalyzer/MPI-Checker/CMakeLists.txt
tools/clang/unittests/StaticAnalyzer/MPI-Checker/ContainerTest.cpp
tools/clang/unittests/StaticAnalyzer/MPI-Checker/UtilityTest.cpp
Index: tools/clang/unittests/StaticAnalyzer/MPI-Checker/UtilityTest.cpp
===
--- tools/clang/unittests/StaticAnalyzer/MPI-Checker/UtilityTest.cpp
+++ tools/clang/unittests/StaticAnalyzer/MPI-Checker/UtilityTest.cpp
@@ -0,0 +1,12 @@
+#include "gtest/gtest.h"
+#include "Utility.h"
+
+TEST(Utility, split) {
+ auto s = util::split("aaa:bbb", ':');
+ EXPECT_EQ(s[0], "aaa");
+ EXPECT_EQ(s[1], "bbb");
+
+ auto s2 = util::split("aaa,bbb", ',');
+ EXPECT_EQ(s2[0], "aaa");
+ EXPECT_EQ(s2[1], "bbb");
+}
Index: tools/clang/unittests/StaticAnalyzer/MPI-Checker/ContainerTest.cpp
===
--- tools/clang/unittests/StaticAnalyzer/MPI-Checker/ContainerTest.cpp
+++ tools/clang/unittests/StaticAnalyzer/MPI-Checker/ContainerTest.cpp
@@ -0,0 +1,10 @@
+#include "gtest/gtest.h"
+#include
+#include "Container.h"
+
+TEST(Container, contains) {
+ std::vector v{0, 1, 2, 3, 4, 5, 6, 8, 9};
+ EXPECT_TRUE(cont::contains(v, 0));
+ EXPECT_TRUE(cont::contains(v, 3));
+ EXPECT_TRUE(cont::contains(v, 9));
+}
Index: tools/clang/unittests/StaticAnalyzer/MPI-Checker/CMakeLists.txt
===
--- tools/clang/unittests/StaticAnalyzer/MPI-Checker/CMakeLists.txt
+++ tools/clang/unittests/StaticAnalyzer/MPI-Checker/CMakeLists.txt
@@ -0,0 +1,18 @@
+set(LLVM_LINK_COMPONENTS Support)
+
+include_directories(
+../../../lib/StaticAnalyzer/Checkers/MPI-Checker
+)
+
+add_clang_unittest(MPI-Checker
+UtilityTest.cpp
+ContainerTest.cpp
+)
+
+target_link_libraries(MPI-Checker
+ clangAST
+ clangBasic
+ clangLex
+ clangParse
+ clangSema
+ clangStaticAnalyzerCheckers)
Index: tools/clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- tools/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ tools/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -6,8 +6,10 @@
AnalyzerOptionsTest.cpp
)
+add_subdirectory(MPI-Checker)
+
target_link_libraries(StaticAnalysisTests
clangBasic
clangAnalysis
- clangStaticAnalyzerCore
+ clangStaticAnalyzerCore
)
Index: tools/clang/test/Analysis/MPIChecker.c
===
--- tools/clang/test/Analysis/MPIChecker.c
+++ tools/clang/test/Analysis/MPIChecker.c
@@ -0,0 +1,456 @@
+// RUN: %clang_cc1 -I/usr/include/ -I/usr/local/include/ -analyze -analyzer-checker=mpi.MPI-Checker -verify %s
+
+#include
+#include
+#include
+
+void doubleWait() {
+ int rank = 0;
+ double buf = 0;
+ MPI_Comm_rank(MPI_COMM_WORLD, &rank);
+
+ if (rank > 0) {
+MPI_Request req[2];
+
+MPI_Isend(&buf, 1, MPI_DOUBLE, rank + 1, 0, MPI_COMM_WORLD, &req[0]);
+MPI_Irecv(&buf, 1, MPI_DOUBLE, rank - 1, 0, MPI_COMM_WORLD, &req[1]);
+
+MPI_Wait(&req[0], MPI_STATUS_IGNORE);
+MPI_Waitall(2, req, MPI_STATUS_IGNORE); // expected-warning{{Request 'req[0]' is already waited upon by 'MPI_Wait' in line 18.}}
+ }
+}
+
+void doubleWait2() {
+ int rank = 0;
+ double buf = 0;
+ MPI_Comm_rank(MPI_COMM_WORLD, &rank);
+ if (rank != 0) {
+MPI_Request sendReq1, recvReq1;
+
Re: recordDecl() AST matcher
On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman wrote: > On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek wrote: > > > > > > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman > > wrote: > >> > >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith > >> wrote: > >> > I don't think CXXRecordDecl is an anachronism, so much as an > >> > implementation > >> > detail; it makes sense to use a smaller class when in C mode, as we > >> > don't > >> > need most of the features and complexity that CXXRecordDecl brings > with > >> > it. > >> > But... as a user of clang matchers, I don't think I'd want to care > about > >> > the > >> > difference, and it'd be more convenient if I could nest (say) a > >> > hasMethod > >> > matcher within a recordDecl matcher, since it's completely obvious > what > >> > that > >> > should mean. If I have a matcher that says: > >> > > >> > recordDecl(or(hasMethod(...), hasField(...))) > >> > > >> > I would expect that to work in both C and C++ (and the only way it > could > >> > match in C would be on a record with the specified field, since the > >> > hasMethod matcher would always fail). > >> > >> Okay, so then it sounds like we want recordDecl to *mean* RecordDecl, > >> but we want the traversal and narrowing matchers that currently take a > >> CXXRecordDecl to instead take a RecordDecl and handle the CXX part > >> transparently? This means we would not need to add a cxxRecordDecl() > >> matcher, but could still access CXX-only functionality (like access > >> control, base classes, etc) through recordDecl()? > > > > > > I'm against that proposal. I think we have tried to make the matchers > more > > "user friendly" in the past, and all those attempts have failed > miserably; > > in the end, users will do ast-dump to see what they want to match, and > then > > be confused when the matchers do follow the AST 99% of the time, but try > to > > be smart 1% of the time. > > I think given that we want to keep CXXRecordDecl, the right solution is > to > > have a cxxRecordDecl() matcher. > > Personally, I think this makes the most sense, at least to me. The > recommendation I've always heard (and given) is to use -ast-dump and > write matchers from there. (Consequently, the more I work with type > traversal matchers, the more I wish we had -ast-dump-types to give > even *more* information for writing matchers.) > > But the question still remains, what do we do with recordDecl? Right > now, it means CXXRecordDecl instead of RecordDecl. If we change it to > mean RecordDecl instead, there's a chance we'll break existing, > reasonable code. Are we okay with that risk? If we're at least > conceptually okay with it, I could make the change locally and see > just how much of our own code breaks, and report back. But if that > turns out to be problematic, do we want to deprecate recordDecl and > replace it with structDecl as our fallback position? Or is there a > better solution? > > Basically, I see a few ways to solve this (and there may be other ways > I'm not thinking about yet): > > 1) Undocument/deprecate recordDecl, add structDecl, unionDecl, and > cxxRecordDecl. This does not match the AST because we have no > StructDecl or UnionDecl types. The more I think about this option, the > less I like it. It's easy to implement, but seems hard to relate to > the AST. > 2) Make recordDecl match RecordDecl, don't touch other matchers. Add > way to distinguish unions from structs (e.g., isUnion(), isStruct()), > add cxxRecordDecl. This matches the AST most closely, but may break > code. I think that I prefer this approach, but it depends heavily on > what "may break code" looks like in practice. > 3) Make recordDecl match RecordDecl, fix other matchers that currently > take CXXRecordDecl to instead take RecordDecl and handle sensibly when > possible. Add a way to distinguish unions from structs, add > cxxRecordDecl. This doesn't match the AST because we will have > matchers taking a RecordDecl when the AST would require a > CXXRecordDecl, but is likely to break less code. > That sums it up. My preferences are 2, 3, 1 in that order :) > ~Aaron > > > > Richard: if CXXRecordDecl was really an implementation detail, it would > be > > hidden behind a RecordDecl class, as an implementation detail. The > reasons > > why we don't want it to be an implementation detail in the code > > (performance, data structure size) don't matter - in the end, it's in the > > AST API. > > > >> > >> > >> ~Aaron > >> > >> > > >> > On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek > >> > wrote: > >> >> > >> >> Richard! We need an informed opinion :D > >> >> > >> >> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman < > aa...@aaronballman.com> > >> >> wrote: > >> >>> > >> >>> Ping? > >> >>> > >> >>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek > >> >>> wrote: > >> >>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman > >> >>> > > >> >>> > wrote: > >> >>> >> > >> >>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek > > >> >>> >> wrote: > >> >>>
