[ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

[vsapsai]

Summary

Because Selector::getIdentifierInfoForSlot returns NULL if a slot has
no corresponding identifier, use Selector::getNameForSlot instead.

rdar://problem/51615164

[rtrieu]

clang/lib/AST/ODRHash.cpp

73–74

There's actually a second bug here as well. When processing an arbitrary number of elements, the number of elements needs to placed before the list. This line should be added before the for-loop:

ID.AddInteger(NumArgs);

This change isn't directly related to your original patch, so feel free to skip it.

76–80

For possibly null pointers, ODRHash uses a boolean before processing the pointer. The way to fix this is:

const IdentifierInfo *II = S.getIdentifierInfoForSlot(i);
AddBoolean(II)
if (II) {
  AddIdentifierInfo(II);
}

[vsapsai]

clang/lib/AST/ODRHash.cpp

73–74

Thanks for the review, Richard. What would be the way to test the suggested changes? I was mostly thinking about making sure we treat as different

• -foo: and -foo::
• -foo:bar:: and -foo::bar:

Does it sound reasonable? Maybe you have some test examples for C++ that I can adopt for Objective-C.

[rtrieu]

clang/lib/AST/ODRHash.cpp

73–74

Yes, checking things close to each other sounds reasonable. I did pretty much the same thing testing in C++, find things (usually types) that are close to others and making sure the ODRHash can tell them apart. The ObjC testing is a little sparse since I was focusing on the C++ part. Thanks for helping fill it out.

[vsapsai]

• Address review comments.

[rtrieu]

LGTM

clang/lib/AST/ODRHash.cpp

case DeclarationName::Identifier:
     break;
   case DeclarationName::ObjCZeroArgSelector:
   case DeclarationName::ObjCOneArgSelector:
   case DeclarationName::ObjCMultiArgSelector: {
     Selector S = Name.getObjCSelector();
     AddBoolean(S.isNull());
     AddBoolean(S.isKeywordSelector());
     AddBoolean(S.isUnarySelector());
     unsigned NumArgs = S.getNumArgs();
+    ID.AddInteger(NumArgs);
     for (unsigned i = 0; i < NumArgs; ++i) {
-      AddIdentifierInfo(S.getIdentifierInfoForSlot(i));
+      const IdentifierInfo *II = S.getIdentifierInfoForSlot(i);
+      AddBoolean(II);
+      if (II) {
+        AddIdentifierInfo(II);
+      }
     }
     break;
   }
   case DeclarationName::CXXConstructorName:
   case DeclarationName::CXXDestructorName:
     AddQualType(Name.getCXXNameType());
     break;
   case DeclarationName::CXXOperatorName:

clang/test/Modules/odr_hash.mm

 @end
 
 @interface Interface2 <T : I1 *>
 @end
 
 @interface Interface3 <T : I1 *>
 @end
 
+@interface EmptySelectorSlot
+- (void)method:(int)arg;
+- (void)method:(int)arg :(int)empty;
+
+- (void)multiple:(int)arg1 args:(int)arg2 :(int)arg3;
+- (void)multiple:(int)arg1 :(int)arg2 args:(int)arg3;
+@end
+
 #endif
 
 #if defined(FIRST)
 struct S {
   Interface1 *I;
   decltype(I->x) x;
   int y;
 };

 Invalid3 i3;
 // expected-error@first.h:* {{'Types::ObjCTypeParam::Invalid3::x' from module 'FirstModule' is not present in definition of 'Types::ObjCTypeParam::Invalid3' in module 'SecondModule'}}
 // expected-note@second.h:* {{declaration of 'x' does not match}}
 #endif
 
 }  // namespace ObjCTypeParam
 }  // namespace Types
 
+namespace CallMethods {
+#if defined(FIRST)
+void invalid1(EmptySelectorSlot *obj) {
+  [obj method:0];
+}
+void invalid2(EmptySelectorSlot *obj) {
+  [obj multiple:0 args:0 :0];
+}
+#elif defined(SECOND)
+void invalid1(EmptySelectorSlot *obj) {
+  [obj method:0 :0];
+}
+void invalid2(EmptySelectorSlot *obj) {
+  [obj multiple:0 :0 args:0];
+}
+#endif
+// expected-error@second.h:* {{'CallMethods::invalid1' has different definitions in different modules; definition in module 'SecondModule' first difference is function body}}
+// expected-note@first.h:* {{but in 'FirstModule' found a different body}}
+
+// expected-error@second.h:* {{'CallMethods::invalid2' has different definitions in different modules; definition in module 'SecondModule' first difference is function body}}
+// expected-note@first.h:* {{but in 'FirstModule' found a different body}}
+}  // namespace CallMethods
+
 // Keep macros contained to one file.
 #ifdef FIRST
 #undef FIRST
 #endif
 
 #ifdef SECOND
 #undef SECOND
 #endif