Page MenuHomePhabricator

[analyzer][www] Update avaible_checks.html
AbandonedPublic

Authored by Szelethus on Oct 10 2018, 2:46 AM.

Details

Summary

Title says it all. I never ever used ObjC, so I couldn't really add examples on many occasions.

These checkers are still in existence that aren't on the website just yet:

  • apiModeling.StdCLibraryFunctions
  • apiModeling.TrustNonnull
  • apiModeling.google.GTest
  • cplusplus.SelfAssignment
  • core.DynamicTypePropagation
  • core.NonnilStringConstants
  • osx.cocoa.NonNilReturnValue

These checkers we don't have a good description for:

  • osx.ObjCProperty

I suspect these are implicit checks, and should be placed in implicit_checks.html, but I'll make sure that that page isn't as well hidden as it is now.

Diff Detail

Event Timeline

Szelethus created this revision.Oct 10 2018, 2:46 AM
Szelethus updated this revision to Diff 168966.Oct 10 2018, 2:53 AM
Szelethus edited the summary of this revision. (Show Details)
Szelethus added a reviewer: rnkovacs.
Szelethus edited the summary of this revision. (Show Details)
Szelethus added inline comments.
www/analyzer/available_checks.html
375–392

@rnkovacs Is this a good description of your checker?

I am not sure what to do about implcit checks. Those are probably should never be turned on or off by the user, but they should be on or off by default based on the set of checks the user enabled and the platform she is using. Thus, I am perfectly ok with the implicit_checks.html only being accessible from the checker development manual. Maybe we should extend the checker list with a notice that the user should never disable the core checks.

Szelethus added a comment.EditedOct 10 2018, 4:00 AM

Thats a great idea.

About implicit checks, they are so well hidden, I didn't even find them until I wanted to update the website (although, this is at least in part my fault, but why would anyone carefully read through a website that hasn't been touched for years?). I think with a big enough warning that it shouldn't be touched by normal users, it should be on this page.

rnkovacs added inline comments.Oct 10 2018, 4:40 AM
www/analyzer/available_checks.html
375–392

Hmm, how about:

void log(const char *str);

void test(int value) {
  const char *msg = std::to_string(value).c_str();
  // msg points to the buffer of a temporary that is now destroyed
  log(msg);  // warn: inner pointer of container used after re/deallocation
}

Most of the issues it found in real code looked like this.
Thanks a lot!

george.karpenkov requested changes to this revision.Oct 10 2018, 10:46 AM

Great idea, thanks!

Should be good to go once examples are added, and implicit checks are removed.

www/analyzer/available_checks.html
781

proper uses is not particularly descriptive?

888

Yep, not very useful without an example. I'm pretty sure the code and tests have one.

1013

Yeah, doesn't seem useful to the user.

This revision now requires changes to proceed.Oct 10 2018, 10:46 AM

Well, the reason why I didn't add tests for these, is that I know so little of ObjC, I am not even sure when a test case begins and ends. I could go ahead and google something about the language, but for a site that advertises to find bugs, maybe someone with more expertise should do the explanation.

Can someone fill those couple bits in? I tried, but fell flat on my face.

www/analyzer/available_checks.html
781

Well, I copied it from the actual checker description :/ I genuinely have no idea how properties work in ObjC, so I can't really update it on my own.

Szelethus updated this revision to Diff 169342.EditedOct 11 2018, 4:48 PM
Szelethus edited the summary of this revision. (Show Details)
  • Removed osx.cocoa.Loops

I still didn't add more description to objc checkers for the reasons stated above.

Szelethus edited the summary of this revision. (Show Details)Oct 11 2018, 4:54 PM

@Szelethus Also you have without a doubt noticed that a "Download" section on the index page could be improved :P

NoQ accepted this revision.Oct 19 2018, 6:07 PM

I guess maybe let's skip stuff without examples and leave Objective-C descriptions waiting on us?

www/analyzer/available_checks.html
482

Wow, i never noticed this one. It seems to contain a syntactic (local) check for the StringRef problem.

george.karpenkov requested changes to this revision.Oct 22 2018, 11:10 AM
george.karpenkov added inline comments.
www/analyzer/available_checks.html
458

Spurious newline

495

If we don't have a description, let's just drop it.

658

I have a longer description:

This checker finds a common performance anti-pattern in a code that uses Grand Central dispatch.
The anti-pattern involves emulating a synchronous call from an asynchronous API using semaphores,
as in the snippet below, where the requestCurrentTaskName function makes an XPC call and then uses the semaphore to
block until the XPC call returns:

+ (NSString *)requestCurrentTaskName {
    __block NSString *taskName = nil;
    dispatch_semaphore_t sema = dispatch_semaphore_create(0);
    NSXPCConnection *connection = [[NSXPCConnection alloc] initWithServiceName:@"MyConnection"];
    id remoteObjectProxy = connection.remoteObjectProxy;
    [remoteObjectProxy requestCurrentTaskName:^(NSString *task) {
        taskName = task;
        dispatch_semaphore_signal(sema);
    }];
    dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER);
    return taskName;
}

Usage of such a pattern in production code running on the main thread is discouraged, as the main queue gets blocked waiting for the background queue,
which could be running at a lower priority, and unnecessary threads are spawned in the process.

In order to avoid the anti-pattern, the available alternatives are:

  • Use the synchronous version of the API, if available.

In the example above, the synchronous XPC proxy object can be used:

+ (NSString *)requestCurrentTaskName {
    __block NSString *taskName = nil;
    NSXPCConnection *connection = [[NSXPCConnection alloc] initWithServiceName:@"MyConnection"];
    id remoteObjectProxy = [connection synchronousRemoteObjectProxyWithErrorHandler:^(NSError *error) {
      NSLog(@"Error = %@", error);

    }];
    [remoteObjectProxy requestCurrentTaskName:^(NSString *task) {
        taskName = task;
    }];
    return taskName;
}
  • Alternatively, the API can be used in the asynchronous way.
782

If we don't have proper description, let's drop.

884

I have a longer description:

Under ARC, function parameters which are pointers to pointers (e.g. NSError**) are __autoreleasing.
Writing to such parameters inside autoreleasing pools might crash whenever the parameter outlives the pool. Detecting such crashes may be difficult, as usage of autorelease pool is usually hidden inside the called functions implementation. Examples include:

BOOL writeToErrorWithIterator(NSError *__autoreleasing* error, NSArray *a) { [a enumerateObjectsUsingBlock:^{
    *error = [NSError errorWithDomain:1];
    }];
}

and

BOOL writeToErrorInBlockFromCFunc(NSError *__autoreleasing* error) {
    dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
    dispatch_async(queue, ^{
        if (error) {
            *error = [NSError errorWithDomain:1];
        }
        dispatch_semaphore_signal(sem);
    });
 
    dispatch_semaphore_wait(sem, 100);
  return 0;
}
1099

Probably should be dropped.

1136

Top of the checker file has a somewhat reasonable description:

A checker for detecting leaks resulting from allocating temporary
autoreleased objects before starting the main run loop.

Checks for two antipatterns:
1. ObjCMessageExpr followed by [[NSRunLoop mainRunLoop] run] in the same
autorelease pool.
2. ObjCMessageExpr followed by [[NSRunLoop mainRunLoop] run] in no
autorelease pool.

Any temporary objects autoreleased in code called in those expressions
// will not be deallocated until the program exits, and are effectively leaks.

This revision now requires changes to proceed.Oct 22 2018, 11:10 AM
Szelethus updated this revision to Diff 170920.Oct 24 2018, 9:37 AM
Szelethus edited the summary of this revision. (Show Details)
Szelethus marked 14 inline comments as done.Oct 24 2018, 9:39 AM
Szelethus added inline comments.
www/analyzer/available_checks.html
458

Actually, in this section of the code, entries are separated with 2 newlines. But it's not super consistent.

495

I added a description, I'd strongly prefer keeping this in.

george.karpenkov requested changes to this revision.Oct 24 2018, 10:24 AM

Good to go provided you will add an example.
If we want to be serious about this page, it really has to be auto-generated (like clang-tidy one), but I understand that this is a larger undertaking.

www/analyzer/available_checks.html
495

Also would need a short example. Usually one can be found in tests.

This revision now requires changes to proceed.Oct 24 2018, 10:24 AM
Szelethus marked 2 inline comments as done.Oct 24 2018, 12:10 PM
Szelethus added inline comments.
www/analyzer/available_checks.html
1136

Should be come up with an example for this one too then?

Szelethus updated this revision to Diff 171624.Oct 29 2018, 6:41 PM

Excuse my informality, but llvm.Conventions fell flat on its face in my eyes (details: D53856), so I'm no longer insisting on including it on this page.

Szelethus abandoned this revision.Nov 18 2018, 3:38 AM

If we want to be serious about this page, it really has to be auto-generated (like clang-tidy one), but I understand that this is a larger undertaking.

Since sphinx is on the way, hopefully, let's just look for a long term solution.

Let's register the ID...

Superseded by D54429.