[analyzer] Remove rdar links from static analyzer and libAnalysis sources. NFC.

I actually visited each link and added relevant context directly to the code.

This is related to the effort to eliminate internal bug tracker links
(d618f1c, e0ac46e).

Test files still have a lot of rdar links and ids in them.
I haven't touched them yet.

Author: haoNoQ

clang/lib/Analysis/CFG.cpp

@@ -2224,8 +2224,7 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc,
       // FIXME: The expression inside a CXXDefaultArgExpr is owned by the
       // called function's declaration, not by the caller. If we simply add
       // this expression to the CFG, we could end up with the same Expr
-      // appearing multiple times.
-      // PR13385 / <rdar://problem/12156507>
+      // appearing multiple times (PR13385).
       //
       // It's likewise possible for multiple CXXDefaultInitExprs for the same
       // expression to be used in the same function (through aggregate

clang/lib/Analysis/RetainSummaryManager.cpp

@@ -301,8 +301,9 @@ const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject(
 
   std::string RetTyName = RetTy.getAsString();
   if (FName == "pthread_create" || FName == "pthread_setspecific") {
-    // Part of: <rdar://problem/7299394> and <rdar://problem/11282706>.
-    // This will be addressed better with IPA.
+    // It's not uncommon to pass a tracked object into the thread
+    // as 'void *arg', and then release it inside the thread.
+    // FIXME: We could build a much more precise model for these functions.
     return getPersistentStopSummary();
   } else if(FName == "NSMakeCollectable") {
     // Handle: id NSMakeCollectable(CFTypeRef)
@@ -311,7 +312,8 @@ const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject(
                                  : getPersistentStopSummary();
   } else if (FName == "CMBufferQueueDequeueAndRetain" ||
              FName == "CMBufferQueueDequeueIfDataReadyAndRetain") {
-    // Part of: <rdar://problem/39390714>.
+    // These API functions are known to NOT act as a CFRetain wrapper.
+    // They simply make a new object owned by the caller.
     return getPersistentSummary(RetEffect::MakeOwned(ObjKind::CF),
                                 ScratchArgs,
                                 ArgEffect(DoNothing),
@@ -324,52 +326,47 @@ const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject(
                FName == "IOServiceNameMatching" ||
                FName == "IORegistryEntryIDMatching" ||
                FName == "IOOpenFirmwarePathMatching"))) {
-    // Part of <rdar://problem/6961230>. (IOKit)
-    // This should be addressed using a API table.
+    // Yes, these IOKit functions return CF objects.
+    // They also violate the CF naming convention.
     return getPersistentSummary(RetEffect::MakeOwned(ObjKind::CF), ScratchArgs,
                                 ArgEffect(DoNothing), ArgEffect(DoNothing));
   } else if (FName == "IOServiceGetMatchingService" ||
              FName == "IOServiceGetMatchingServices") {
-    // FIXES: <rdar://problem/6326900>
-    // This should be addressed using a API table.  This strcmp is also
-    // a little gross, but there is no need to super optimize here.
+    // These IOKit functions accept CF objects as arguments.
+    // They also consume them without an appropriate annotation.
     ScratchArgs = AF.add(ScratchArgs, 1, ArgEffect(DecRef, ObjKind::CF));
     return getPersistentSummary(RetEffect::MakeNoRet(),
                                 ScratchArgs,
                                 ArgEffect(DoNothing), ArgEffect(DoNothing));
   } else if (FName == "IOServiceAddNotification" ||
              FName == "IOServiceAddMatchingNotification") {
-    // Part of <rdar://problem/6961230>. (IOKit)
-    // This should be addressed using a API table.
+    // More IOKit functions suddenly accepting (and even more suddenly,
+    // consuming) CF objects.
     ScratchArgs = AF.add(ScratchArgs, 2, ArgEffect(DecRef, ObjKind::CF));
     return getPersistentSummary(RetEffect::MakeNoRet(),
                                 ScratchArgs,
                                 ArgEffect(DoNothing), ArgEffect(DoNothing));
   } else if (FName == "CVPixelBufferCreateWithBytes") {
-    // FIXES: <rdar://problem/7283567>
     // Eventually this can be improved by recognizing that the pixel
     // buffer passed to CVPixelBufferCreateWithBytes is released via
     // a callback and doing full IPA to make sure this is done correctly.
-    // FIXME: This function has an out parameter that returns an
+    // Note that it's passed as a 'void *', so it's hard to annotate.
+    // FIXME: This function also has an out parameter that returns an
     // allocated object.
     ScratchArgs = AF.add(ScratchArgs, 7, ArgEffect(StopTracking));
     return getPersistentSummary(RetEffect::MakeNoRet(),
                                 ScratchArgs,
                                 ArgEffect(DoNothing), ArgEffect(DoNothing));
   } else if (FName == "CGBitmapContextCreateWithData") {
-    // FIXES: <rdar://problem/7358899>
+    // This is similar to the CVPixelBufferCreateWithBytes situation above.
     // Eventually this can be improved by recognizing that 'releaseInfo'
     // passed to CGBitmapContextCreateWithData is released via
     // a callback and doing full IPA to make sure this is done correctly.
     ScratchArgs = AF.add(ScratchArgs, 8, ArgEffect(ArgEffect(StopTracking)));
     return getPersistentSummary(RetEffect::MakeOwned(ObjKind::CF), ScratchArgs,
                                 ArgEffect(DoNothing), ArgEffect(DoNothing));
   } else if (FName == "CVPixelBufferCreateWithPlanarBytes") {
-    // FIXES: <rdar://problem/7283567>
-    // Eventually this can be improved by recognizing that the pixel
-    // buffer passed to CVPixelBufferCreateWithPlanarBytes is released
-    // via a callback and doing full IPA to make sure this is done
-    // correctly.
+    // Same as CVPixelBufferCreateWithBytes, just more arguments.
     ScratchArgs = AF.add(ScratchArgs, 12, ArgEffect(StopTracking));
     return getPersistentSummary(RetEffect::MakeNoRet(),
                                 ScratchArgs,
@@ -386,12 +383,10 @@ const RetainSummary *RetainSummaryManager::getSummaryForObjCOrCFObject(
                                 ArgEffect(DoNothing), ArgEffect(DoNothing));
   } else if (FName == "dispatch_set_context" ||
              FName == "xpc_connection_set_context") {
-    // <rdar://problem/11059275> - The analyzer currently doesn't have
-    // a good way to reason about the finalizer function for libdispatch.
+    // The analyzer currently doesn't have a good way to reason about
+    // dispatch_set_finalizer_f() which typically cleans up the context.
     // If we pass a context object that is memory managed, stop tracking it.
-    // <rdar://problem/13783514> - Same problem, but for XPC.
-    // FIXME: this hack should possibly go away once we can handle
-    // libdispatch and XPC finalizers.
+    // Same with xpc_connection_set_finalizer_f().
     ScratchArgs = AF.add(ScratchArgs, 1, ArgEffect(StopTracking));
     return getPersistentSummary(RetEffect::MakeNoRet(),
                                 ScratchArgs,
@@ -740,8 +735,8 @@ RetainSummaryManager::canEval(const CallExpr *CE, const FunctionDecl *FD,
     // It's okay to be a little sloppy here.
     if (FName == "CMBufferQueueDequeueAndRetain" ||
         FName == "CMBufferQueueDequeueIfDataReadyAndRetain") {
-      // Part of: <rdar://problem/39390714>.
-      // These are not retain. They just return something and retain it.
+      // These API functions are known to NOT act as a CFRetain wrapper.
+      // They simply make a new object owned by the caller.
       return std::nullopt;
     }
     if (CE->getNumArgs() == 1 &&
@@ -1243,8 +1238,6 @@ void RetainSummaryManager::InitializeMethodSummaries() {
   // FIXME: For now we opt for false negatives with NSWindow, as these objects
   //  self-own themselves.  However, they only do this once they are displayed.
   //  Thus, we need to track an NSWindow's display status.
-  //  This is tracked in <rdar://problem/6062711>.
-  //  See also http://llvm.org/bugs/show_bug.cgi?id=3714.
   const RetainSummary *NoTrackYet =
       getPersistentSummary(RetEffect::MakeNoRet(), ScratchArgs,
                            ArgEffect(StopTracking), ArgEffect(StopTracking));
@@ -1259,7 +1252,6 @@ void RetainSummaryManager::InitializeMethodSummaries() {
 
   // For NSNull, objects returned by +null are singletons that ignore
   // retain/release semantics.  Just don't track them.
-  // <rdar://problem/12858915>
   addClassMethSummary("NSNull", "null", NoTrackYet);
 
   // Don't track allocated autorelease pools, as it is okay to prematurely

clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

@@ -219,7 +219,6 @@ void WalkAST::VisitForStmt(ForStmt *FS) {
 
 //===----------------------------------------------------------------------===//
 // Check: floating point variable used as loop counter.
-// Originally: <rdar://problem/6336718>
 // Implements: CERT security coding advisory FLP-30.
 //===----------------------------------------------------------------------===//
 
@@ -467,8 +466,8 @@ void WalkAST::checkCall_bzero(const CallExpr *CE, const FunctionDecl *FD) {
 
 
 //===----------------------------------------------------------------------===//
-// Check: Any use of 'gets' is insecure.
-// Originally: <rdar://problem/6335715>
+// Check: Any use of 'gets' is insecure. Most man pages literally says this.
+//
 // Implements (part of): 300-BSI (buildsecurityin.us-cert.gov)
 // CWE-242: Use of Inherently Dangerous Function
 //===----------------------------------------------------------------------===//
@@ -847,8 +846,13 @@ bool WalkAST::checkCall_strCommon(const CallExpr *CE, const FunctionDecl *FD) {
 }
 
 //===----------------------------------------------------------------------===//
-// Check: Linear congruent random number generators should not be used
-// Originally: <rdar://problem/63371000>
+// Check: Linear congruent random number generators should not be used,
+// i.e. rand(), random().
+//
+// E. Bach, "Efficient prediction of Marsaglia-Zaman random number generators,"
+// in IEEE Transactions on Information Theory, vol. 44, no. 3, pp. 1253-1257,
+// May 1998, https://doi.org/10.1109/18.669305
+//
 // CWE-338: Use of cryptographically weak prng
 //===----------------------------------------------------------------------===//
 
@@ -890,11 +894,7 @@ void WalkAST::checkCall_rand(const CallExpr *CE, const FunctionDecl *FD) {
                      CE->getCallee()->getSourceRange());
 }
 
-//===----------------------------------------------------------------------===//
-// Check: 'random' should not be used
-// Originally: <rdar://problem/63371000>
-//===----------------------------------------------------------------------===//
-
+// See justification for rand().
 void WalkAST::checkCall_random(const CallExpr *CE, const FunctionDecl *FD) {
   if (!CheckRand || !filter.check_rand)
     return;
@@ -990,8 +990,18 @@ void WalkAST::checkMsg_decodeValueOfObjCType(const ObjCMessageExpr *ME) {
 }
 
 //===----------------------------------------------------------------------===//
-// Check: Should check whether privileges are dropped successfully.
-// Originally: <rdar://problem/6337132>
+// Check: The caller should always verify that the privileges
+// were dropped successfully.
+//
+// Some library functions, like setuid() and setgid(), should always be used
+// with a check of the return value to verify that the function completed
+// successfully.  If the drop fails, the software will continue to run
+// with the raised privileges, which might provide additional access
+// to unprivileged users.
+//
+// (Note that this check predates __attribute__((warn_unused_result)).
+// Do we still need it now that we have a compiler warning for this?
+// Are these standard functions already annotated this way?)
 //===----------------------------------------------------------------------===//
 
 void WalkAST::checkUncheckedReturnValue(CallExpr *CE) {

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

@@ -786,9 +786,6 @@ RefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
   assert(RV);
 
   if (RV->getKind() == RefVal::ErrorLeakReturned) {
-    // FIXME: Per comments in rdar://6320065, "create" only applies to CF
-    // objects.  Only "copy", "alloc", "retain" and "new" transfer ownership
-    // to the caller for NS objects.
     const Decl *D = &EndN->getCodeDecl();
 
     os << (isa<ObjCMethodDecl>(D) ? " is returned from a method "

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

@@ -765,8 +765,9 @@ RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
     // the static type. However, because we currently don't update
     // DynamicTypeInfo when an object is cast, we can't actually be sure the
     // DynamicTypeInfo is up to date. This assert should be re-enabled once
-    // this is fixed. <rdar://problem/12287087>
-    //assert(!MD->getParent()->isDerivedFrom(RD) && "Bad DynamicTypeInfo");
+    // this is fixed.
+    //
+    // assert(!MD->getParent()->isDerivedFrom(RD) && "Bad DynamicTypeInfo");
 
     return {};
   }

clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

@@ -167,19 +167,32 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME,
   // intentionally drops coverage in order to prevent false alarms
   // in the following scenario:
   //
-  // id result = [o someMethod]
-  // if (result) {
-  //   if (!o) {
-  //     // <-- This program point should be unreachable because if o is nil
-  //     // it must the case that result is nil as well.
+  //   id result = [o someMethod]
+  //   if (result) {
+  //     if (!o) {
+  //       // <-- This program point should be unreachable because if o is nil
+  //       // it must the case that result is nil as well.
+  //     }
   //   }
-  // }
   //
-  // We could avoid dropping coverage by performing an explicit case split
-  // on each method call -- but this would get very expensive. An alternative
-  // would be to introduce lazy constraints.
-  // FIXME: This ignores many potential bugs (<rdar://problem/11733396>).
-  // Revisit once we have lazier constraints.
+  // However, it also loses coverage of the nil path prematurely,
+  // leading to missed reports.
+  //
+  // It's possible to handle this by performing a state split on every call:
+  // explore the state where the receiver is non-nil, and independently
+  // explore the state where it's nil. But this is not only slow, but
+  // completely unwarranted. The mere presence of the message syntax in the code
+  // isn't sufficient evidence that nil is a realistic possibility.
+  //
+  // An ideal solution would be to add the following constraint that captures
+  // both possibilities without splitting the state:
+  //
+  //   ($x == 0) => ($y == 0)                                                (1)
+  //
+  // where in our case '$x' is the receiver symbol, '$y' is the returned symbol,
+  // and '=>' is logical implication. But RangeConstraintManager can't handle
+  // such constraints yet, so for now we go with a simpler, more restrictive
+  // constraint: $x != 0, from which (1) follows as a vacuous truth.
   if (Msg->isInstanceMessage()) {
     SVal recVal = Msg->getReceiverSVal();
     if (!recVal.isUndef()) {