r333316 - Support Swift calling convention for PPC64 targets

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Fri May 25 14:26:03 2018
New Revision: 16

URL: http://llvm.org/viewvc/llvm-project?rev=16=rev
Log:
Support Swift calling convention for PPC64 targets

This adds basic support for the Swift calling convention with PPC64 targets.
Patch provided by Atul Sowani in bug report #37223

Modified:
cfe/trunk/lib/Basic/Targets/PPC.h
cfe/trunk/lib/CodeGen/TargetInfo.cpp

Modified: cfe/trunk/lib/Basic/Targets/PPC.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/PPC.h?rev=16=15=16=diff
==
--- cfe/trunk/lib/Basic/Targets/PPC.h (original)
+++ cfe/trunk/lib/Basic/Targets/PPC.h Fri May 25 14:26:03 2018
@@ -335,6 +335,15 @@ public:
 }
 return false;
   }
+
+  CallingConvCheckResult checkCallingConvention(CallingConv CC) const override 
{
+switch (CC) {
+case CC_Swift:
+  return CCCR_OK;
+default:
+  return CCCR_Warning;
+}
+  }
 };
 
 class LLVM_LIBRARY_VISIBILITY DarwinPPC32TargetInfo

Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=16=15=16=diff
==
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Fri May 25 14:26:03 2018
@@ -4287,7 +4287,7 @@ PPC32TargetCodeGenInfo::initDwarfEHRegSi
 
 namespace {
 /// PPC64_SVR4_ABIInfo - The 64-bit PowerPC ELF (SVR4) ABI information.
-class PPC64_SVR4_ABIInfo : public ABIInfo {
+class PPC64_SVR4_ABIInfo : public SwiftABIInfo {
 public:
   enum ABIKind {
 ELFv1 = 0,
@@ -4331,7 +4331,7 @@ private:
 public:
   PPC64_SVR4_ABIInfo(CodeGen::CodeGenTypes , ABIKind Kind, bool HasQPX,
  bool SoftFloatABI)
-  : ABIInfo(CGT), Kind(Kind), HasQPX(HasQPX),
+  : SwiftABIInfo(CGT), Kind(Kind), HasQPX(HasQPX),
 IsSoftFloatABI(SoftFloatABI) {}
 
   bool isPromotableTypeForABI(QualType Ty) const;
@@ -4374,6 +4374,15 @@ public:
 
   Address EmitVAArg(CodeGenFunction , Address VAListAddr,
 QualType Ty) const override;
+
+  bool shouldPassIndirectlyForSwift(ArrayRef scalars,
+bool asReturnValue) const override {
+return occupiesMoreThan(CGT, scalars, /*total*/ 4);
+  }
+
+  bool isSwiftErrorInRegister() const override {
+return false;
+  }
 };
 
 class PPC64_SVR4_TargetCodeGenInfo : public TargetCodeGenInfo {


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r310622 - Add a getName accessor for ModuleMacros.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Thu Aug 10 09:42:46 2017
New Revision: 310622

URL: http://llvm.org/viewvc/llvm-project?rev=310622=rev
Log:
Add a getName accessor for ModuleMacros.

Swift would like to be able to access the name of a ModuleMacro.
There was some discussion of this in
https://github.com/apple/swift-clang/pull/93, suggesting that it makes
sense to have this accessor in Clang.

Modified:
cfe/trunk/include/clang/Lex/MacroInfo.h

Modified: cfe/trunk/include/clang/Lex/MacroInfo.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/MacroInfo.h?rev=310622=310621=310622=diff
==
--- cfe/trunk/include/clang/Lex/MacroInfo.h (original)
+++ cfe/trunk/include/clang/Lex/MacroInfo.h Thu Aug 10 09:42:46 2017
@@ -510,6 +510,9 @@ public:
 ID.AddPointer(II);
   }
 
+  /// Get the name of the macro.
+  IdentifierInfo *getName() const { return II; }
+
   /// Get the ID of the module that exports this macro.
   Module *getOwningModule() const { return OwningModule; }
 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r285428 - Add missing newline at EOF to avoid -Wnewline-eof warnings.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Fri Oct 28 13:55:50 2016
New Revision: 285428

URL: http://llvm.org/viewvc/llvm-project?rev=285428=rev
Log:
Add missing newline at EOF to avoid -Wnewline-eof warnings.

Modified:
cfe/trunk/include/clang/Basic/OpenCLImageTypes.def

Modified: cfe/trunk/include/clang/Basic/OpenCLImageTypes.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/OpenCLImageTypes.def?rev=285428=285427=285428=diff
==
--- cfe/trunk/include/clang/Basic/OpenCLImageTypes.def (original)
+++ cfe/trunk/include/clang/Basic/OpenCLImageTypes.def Fri Oct 28 13:55:50 2016
@@ -79,4 +79,4 @@ IMAGE_READ_WRITE_TYPE(image3d, OCLImage3
 #undef GENERIC_IMAGE_TYPE
 #undef IMAGE_READ_TYPE
 #undef IMAGE_WRITE_TYPE
-#undef IMAGE_READ_WRITE_TYPE
\ No newline at end of file
+#undef IMAGE_READ_WRITE_TYPE


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r284265 - [Sema] Refactor context checking for availability diagnostics

[Bob Wilson via cfe-commits]

Yes. I filed rdar://problem/28812809  to report that 
problem in the SDK. I also filed rdar://problem/28825862 
 to remind us to restore the more strict checking 
after we release a version of the tvOS SDK with a fix.

Thanks, Erik!

> On Oct 18, 2016, at 9:46 AM, Erik Pilkington  
> wrote:
> 
> 
> On second thought, I think the header *is* ill-formed here. If we allowed 
> this behaviour, this would provide a way to circumvent an availability 
> diagnostic. For example, now the following compiles cleanly, where we really 
> should emit a diagnostic somewhere!
> 
> typedef int unavail_int __attribute__((availability(tvos, unavailable)));
> 
> __attribute__((availability(tvos, unavailable)))
> @interface A
> extern unavail_int foo;
> @end
> 
> int main() { (void)foo; }
> 
> I’m not so sure about the politics of this, but could you file a radar or 
> something to get the header fixed (i.e, make the variable __TVOS_PROHIBITED)? 
> I’ll make a new patch that makes this a special case for now.
> 
> Sorry for the flip-flop!
> Erik
> 
>> On Oct 18, 2016, at 11:47 AM, Erik Pilkington > > wrote:
>> 
>> Hi Bob,
>> I think the code in the header is fine here, so I reverted in r284486. 
>> Here’s a reduced version:
>> 
>> typedef int unavail_int __attribute__((availability(tvos, unavailable)));
>> 
>> __attribute__((availability(tvos, unavailable)))
>> @interface A
>> extern unavail_int foo;
>> @end
>> 
>> The problem is that ‘foo’ is actually at file context, not in the context of 
>> the interface, because we temporarily switched contexts to parse it. This 
>> means we can’t just look at DeclContexts in the DelayedDiagnostic case, 
>> which is what I was doing here. I’ll try to get a fix out for this soon!
>> 
>> Thanks for pointing this out!
>> Erik
>> 
>>> On Oct 18, 2016, at 1:37 AM, Bob Wilson >> > wrote:
>>> 
>>> Hi Erik,
>>> 
>>> This change does not work with one of the headers from the AVFoundation 
>>> framework in tvOS 10.0. We can try to get a fix into the tvOS SDK, but it 
>>> will probably be a while before we could release an SDK with that change. 
>>> In the meantime, this is kind of disruptive. Can you find a way to keep the 
>>> previous behavior, at least until we have a chance to fix that header?
>>> 
>>> The header in question is 
>>> System/Library/Frameworks/AVFoundation.framework/Headers/AVCaptureDevice.h 
>>> in the AppleTVSimulator.sdk directory from Xcode 8.0. The problematic 
>>> declaration is this one:
>>> 
>>> AVF_EXPORT const AVCaptureWhiteBalanceGains 
>>> AVCaptureWhiteBalanceGainsCurrent NS_AVAILABLE_IOS(8_0);
>>> 
>>> The problem is that the type is declared like this:
>>> 
>>> typedef struct {
>>>float redGain;
>>>float greenGain;
>>>float blueGain;
>>> } AVCaptureWhiteBalanceGains NS_AVAILABLE_IOS(8_0) __TVOS_PROHIBITED;
>>> 
>>> The variable is missing the __TVOS_PROHIBITED attribute. You can reproduce 
>>> this easily:
>>> 
>>> $ cat t.m
>>> #import 
>>> $ clang -c -arch x86_64 -mtvos-simulator-version-min=10.0 -isysroot 
>>> /Applications/Xcode-8.0.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator.sdk
>>>  t.m
>>> /Applications/Xcode-8.0.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator.sdk/System/Library/Frameworks/AVFoundation.framework/Headers/AVCaptureDevice.h:1184:14:
>>>  error: 
>>>  'AVCaptureWhiteBalanceGains' is unavailable: not available on tvOS
>>> extern const AVCaptureWhiteBalanceGains AVCaptureWhiteBalanceGainsCurren...
>>> ^
>>> /Applications/Xcode-8.0.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator.sdk/System/Library/Frameworks/AVFoundation.framework/Headers/AVCaptureDevice.h:1081:3:
>>>  note: 
>>>  'AVCaptureWhiteBalanceGains' has been explicitly marked unavailable 
>>> here
>>> } AVCaptureWhiteBalanceGains __attribute__((availability(ios,introduced=...
>>>  ^
>>> 1 error generated.
>>> 
>>> Maybe we can carve out an exception based on the fact that this is just an 
>>> extern declaration of the variable — it’s not actually being used here. It 
>>> is also defined within the @interface for an Objective-C class, in case 
>>> that helps at all.
>>> 
 On Oct 14, 2016, at 12:08 PM, Erik Pilkington via cfe-commits 
 > wrote:
 
 Author: epilk
 Date: Fri Oct 14 14:08:01 2016
 New Revision: 284265
 
 URL: http://llvm.org/viewvc/llvm-project?rev=284265=rev 
 
 Log:
 [Sema] Refactor context checking for availability diagnostics
 
 This commit combines a couple of redundant functions that do availability
 attribute context checking into a more correct/simpler one.
 
 Differential revision: 

Re: r284265 - [Sema] Refactor context checking for availability diagnostics

[Bob Wilson via cfe-commits]

Hi Erik,

This change does not work with one of the headers from the AVFoundation 
framework in tvOS 10.0. We can try to get a fix into the tvOS SDK, but it will 
probably be a while before we could release an SDK with that change. In the 
meantime, this is kind of disruptive. Can you find a way to keep the previous 
behavior, at least until we have a chance to fix that header?

The header in question is 
System/Library/Frameworks/AVFoundation.framework/Headers/AVCaptureDevice.h in 
the AppleTVSimulator.sdk directory from Xcode 8.0. The problematic declaration 
is this one:

AVF_EXPORT const AVCaptureWhiteBalanceGains AVCaptureWhiteBalanceGainsCurrent 
NS_AVAILABLE_IOS(8_0);

The problem is that the type is declared like this:

typedef struct {
float redGain;
float greenGain;
float blueGain;
} AVCaptureWhiteBalanceGains NS_AVAILABLE_IOS(8_0) __TVOS_PROHIBITED;

The variable is missing the __TVOS_PROHIBITED attribute. You can reproduce this 
easily:

$ cat t.m
#import 
$ clang -c -arch x86_64 -mtvos-simulator-version-min=10.0 -isysroot 
/Applications/Xcode-8.0.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator.sdk
 t.m
/Applications/Xcode-8.0.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator.sdk/System/Library/Frameworks/AVFoundation.framework/Headers/AVCaptureDevice.h:1184:14:
 error: 
  'AVCaptureWhiteBalanceGains' is unavailable: not available on tvOS
extern const AVCaptureWhiteBalanceGains AVCaptureWhiteBalanceGainsCurren...
 ^
/Applications/Xcode-8.0.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator.sdk/System/Library/Frameworks/AVFoundation.framework/Headers/AVCaptureDevice.h:1081:3:
 note: 
  'AVCaptureWhiteBalanceGains' has been explicitly marked unavailable here
} AVCaptureWhiteBalanceGains __attribute__((availability(ios,introduced=...
  ^
1 error generated.

Maybe we can carve out an exception based on the fact that this is just an 
extern declaration of the variable — it’s not actually being used here. It is 
also defined within the @interface for an Objective-C class, in case that helps 
at all.

> On Oct 14, 2016, at 12:08 PM, Erik Pilkington via cfe-commits 
>  wrote:
> 
> Author: epilk
> Date: Fri Oct 14 14:08:01 2016
> New Revision: 284265
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=284265=rev
> Log:
> [Sema] Refactor context checking for availability diagnostics
> 
> This commit combines a couple of redundant functions that do availability
> attribute context checking into a more correct/simpler one.
> 
> Differential revision: https://reviews.llvm.org/D25283
> 
> Modified:
>cfe/trunk/include/clang/Sema/Sema.h
>cfe/trunk/lib/Sema/SemaDecl.cpp
>cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>cfe/trunk/lib/Sema/SemaExpr.cpp
>cfe/trunk/test/SemaObjC/class-unavail-warning.m
> 
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=284265=284264=284265=diff
> ==
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Fri Oct 14 14:08:01 2016
> @@ -9889,23 +9889,16 @@ public:
> return OriginalLexicalContext ? OriginalLexicalContext : CurContext;
>   }
> 
> -  AvailabilityResult getCurContextAvailability() const;
> -
> -  /// \brief Get the verison that this context implies.
> -  /// For instance, a method in an interface that is annotated with an
> -  /// availability attribuite effectively has the availability of the 
> interface.
> -  VersionTuple getVersionForDecl(const Decl *Ctx) const;
> -
>   /// \brief The diagnostic we should emit for \c D, or \c AR_Available.
>   ///
>   /// \param D The declaration to check. Note that this may be altered to 
> point
>   /// to another declaration that \c D gets it's availability from. i.e., we
>   /// walk the list of typedefs to find an availability attribute.
>   ///
> -  /// \param ContextVersion The version to compare availability against.
> -  AvailabilityResult
> -  ShouldDiagnoseAvailabilityOfDecl(NamedDecl *, VersionTuple 
> ContextVersion,
> -   std::string *Message);
> +  /// \param Message If non-null, this will be populated with the message 
> from
> +  /// the availability attribute that is selected.
> +  AvailabilityResult ShouldDiagnoseAvailabilityOfDecl(NamedDecl *,
> +  std::string *Message);
> 
>   const DeclContext *getCurObjCLexicalContext() const {
> const DeclContext *DC = getCurLexicalContext();
> 
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=284265=284264=284265=diff
> ==
> --- 

r275905 - Allow iOS and tvOS version numbers with 2-digit major version numbers.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Mon Jul 18 15:29:14 2016
New Revision: 275905

URL: http://llvm.org/viewvc/llvm-project?rev=275905=rev
Log:
Allow iOS and tvOS version numbers with 2-digit major version numbers.

rdar://problem/26921601

Modified:
cfe/trunk/lib/Basic/Targets.cpp
cfe/trunk/lib/Driver/ToolChains.cpp
cfe/trunk/test/Frontend/darwin-version.c

Modified: cfe/trunk/lib/Basic/Targets.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=275905=275904=275905=diff
==
--- cfe/trunk/lib/Basic/Targets.cpp (original)
+++ cfe/trunk/lib/Basic/Targets.cpp Mon Jul 18 15:29:14 2016
@@ -158,14 +158,25 @@ static void getDarwinDefines(MacroBuilde
 
   // Set the appropriate OS version define.
   if (Triple.isiOS()) {
-assert(Maj < 10 && Min < 100 && Rev < 100 && "Invalid version!");
-char Str[6];
-Str[0] = '0' + Maj;
-Str[1] = '0' + (Min / 10);
-Str[2] = '0' + (Min % 10);
-Str[3] = '0' + (Rev / 10);
-Str[4] = '0' + (Rev % 10);
-Str[5] = '\0';
+assert(Maj < 100 && Min < 100 && Rev < 100 && "Invalid version!");
+char Str[7];
+if (Maj < 10) {
+  Str[0] = '0' + Maj;
+  Str[1] = '0' + (Min / 10);
+  Str[2] = '0' + (Min % 10);
+  Str[3] = '0' + (Rev / 10);
+  Str[4] = '0' + (Rev % 10);
+  Str[5] = '\0';
+} else {
+  // Handle versions >= 10.
+  Str[0] = '0' + (Maj / 10);
+  Str[1] = '0' + (Maj % 10);
+  Str[2] = '0' + (Min / 10);
+  Str[3] = '0' + (Min % 10);
+  Str[4] = '0' + (Rev / 10);
+  Str[5] = '0' + (Rev % 10);
+  Str[6] = '\0';
+}
 if (Triple.isTvOS())
   Builder.defineMacro("__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__", Str);
 else

Modified: cfe/trunk/lib/Driver/ToolChains.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains.cpp?rev=275905=275904=275905=diff
==
--- cfe/trunk/lib/Driver/ToolChains.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains.cpp Mon Jul 18 15:29:14 2016
@@ -687,13 +687,13 @@ void Darwin::AddDeploymentTarget(Derived
 assert(iOSVersion && "Unknown target platform!");
 if (!Driver::GetReleaseVersion(iOSVersion->getValue(), Major, Minor, Micro,
HadExtra) ||
-HadExtra || Major >= 10 || Minor >= 100 || Micro >= 100)
+HadExtra || Major >= 100 || Minor >= 100 || Micro >= 100)
   getDriver().Diag(diag::err_drv_invalid_version_number)
   << iOSVersion->getAsString(Args);
   } else if (Platform == TvOS) {
 if (!Driver::GetReleaseVersion(TvOSVersion->getValue(), Major, Minor,
Micro, HadExtra) || HadExtra ||
-Major >= 10 || Minor >= 100 || Micro >= 100)
+Major >= 100 || Minor >= 100 || Micro >= 100)
   getDriver().Diag(diag::err_drv_invalid_version_number)
   << TvOSVersion->getAsString(Args);
   } else if (Platform == WatchOS) {

Modified: cfe/trunk/test/Frontend/darwin-version.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Frontend/darwin-version.c?rev=275905=275904=275905=diff
==
--- cfe/trunk/test/Frontend/darwin-version.c (original)
+++ cfe/trunk/test/Frontend/darwin-version.c Mon Jul 18 15:29:14 2016
@@ -10,6 +10,8 @@
 // RUN: %clang_cc1 -triple armv6-apple-ios2.3.1 -dM -E -o %t %s
 // RUN: grep '__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__' %t | grep 
'20301' | count 1
 // RUN: not grep '__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__' %t
+// RUN: %clang_cc1 -triple armv7-apple-ios10.1.2 -dM -E -o %t %s
+// RUN: grep '__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__' %t | grep 
'100102' | count 1
 // RUN: %clang_cc1 -triple i386-apple-macosx10.4.0 -dM -E -o %t %s
 // RUN: grep '__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__' %t | grep '1040' 
| count 1
 // RUN: not grep '__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__' %t
@@ -32,6 +34,8 @@
 // RUN: grep '__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__' %t | grep '80300' | 
count 1
 // RUN: not grep '__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__' %t
 // RUN: not grep '__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__' %t
+// RUN: %clang_cc1 -triple arm64-apple-tvos10.2.3 -dM -E -o %t %s
+// RUN: grep '__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__' %t | grep '100203' | 
count 1
 
 // RUN: %clang_cc1 -triple x86_64-apple-tvos8.3 -dM -E -o %t %s
 // RUN: grep '__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__' %t | grep '80300' | 
count 1


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] arc-repeated-use-of-weak should not warn about IBOutlet properties

[Bob Wilson via cfe-commits]

> On May 24, 2016, at 11:46 AM, Manman Ren  wrote:
> 
>> 
>> On May 24, 2016, at 11:42 AM, Bob Wilson > > wrote:
>> 
>>> 
>>> On May 24, 2016, at 11:21 AM, Manman >> > wrote:
>>> 
>>> 
 On May 23, 2016, at 8:15 PM, Bob Wilson > wrote:
 
 Revision r211132 was supposed to disable -Warc-repeated-use-of-weak for 
 Objective-C properties marked with the IBOutlet attribute. Those 
 properties are supposed to be weak but they are only accessed from the 
 main thread so there is no risk of asynchronous updates setting them to 
 nil. That combination makes -Warc-repeated-use-of-weak very noisy. The 
 previous change only handled one kind of access to weak IBOutlet 
 properties. Instead of trying to add checks for all the different kinds of 
 property accesses, this patch removes the previous special case check and 
 adds a check at the point where the diagnostic is reported.
>>> 
>>> The approach looks good to me in general. 
 diff --git lib/Sema/AnalysisBasedWarnings.cpp 
 lib/Sema/AnalysisBasedWarnings.cpp
 index 3f2c41b..eb45315 100644
 --- lib/Sema/AnalysisBasedWarnings.cpp
 +++ lib/Sema/AnalysisBasedWarnings.cpp
 @@ -1334,6 +1334,12 @@ static void diagnoseRepeatedUseOfWeak(Sema ,
else
  llvm_unreachable("Unexpected weak object kind!");
 
 +// Do not warn about IBOutlet weak property receivers being set to 
 null
 +// since they are typically only used from the main thread.
 +if (const ObjCPropertyDecl *Prop = dyn_cast(D))
 +  if (Prop->hasAttr())
 +continue;
 +
// Show the first time the object was read.
S.Diag(FirstRead->getLocStart(), DiagKind)
  << int(ObjectKind) << D << int(FunctionKind)
>>> 
>>> Should we guard the call to diagnoseRepeatedUseOfWeak, instead of checking 
>>> the decl inside a loop in diagnoseRepeatedUseOfWeak?
>>> 
>>> if (S.getLangOpts().ObjCWeak &&
>>> !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, D->getLocStart()))
>>>   diagnoseRepeatedUseOfWeak(S, fscope, D, AC.getParentMap());
>>> —> check IBOutlet here?
>> 
>> diagnoseRepeatedUseOfWeak is used to report all of the issues within a 
>> function. Some of those may involve IBOutlet properties and others not. We 
>> have to check each one separately.
>> 
>> Your comment prompted me to look more closely and I realized that the code 
>> is confusing because there is a local variable “D” that shadows the “const 
>> Decl *D” argument of diagnoseRepeatedUseOfWeak:
>> 
>>const NamedDecl *D = Key.getProperty();
>> 
>> We should rename that to avoid potential confusion. I can do that as a 
>> follow-up change.
> 
> Yes, I missed the local variable “D”.
> 
>> 
>>> 
 diff --git lib/Sema/SemaPseudoObject.cpp lib/Sema/SemaPseudoObject.cpp
 index 62c823b..c93d800 100644
 --- lib/Sema/SemaPseudoObject.cpp
 +++ lib/Sema/SemaPseudoObject.cpp
 @@ -578,7 +578,7 @@ bool ObjCPropertyOpBuilder::isWeakProperty() const {
  if (RefExpr->isExplicitProperty()) {
const ObjCPropertyDecl *Prop = RefExpr->getExplicitProperty();
if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak)
 -  return !Prop->hasAttr();
 +  return true;
 
T = Prop->getType();
  } else if (Getter) {
>>> 
>>> I wonder if this change is necessary to make the testing case pass, or is 
>>> it introduced for clarity, to better reflect the function name 
>>> isWeakProperty?
>> 
>> This is the one special-case check that was introduced by r211132. I removed 
>> it because there is no need for it after we add the check in 
>> diagnoseRepeatedUseOfWeak.
> 
> Thanks for the explanation!
> 
> Patch looks good to me,
> Manman

Thanks for the review.
Patch is in r270665.
I renamed the variable to fix the parameter shadowing in r270666.

> 
>> 
>>> 
>>> Thanks,
>>> Manman
>>> 
 rdar://problem/21366461 
 
 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r270665 - arc-repeated-use-of-weak should not warn about IBOutlet properties

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Wed May 25 00:41:57 2016
New Revision: 270665

URL: http://llvm.org/viewvc/llvm-project?rev=270665=rev
Log:
arc-repeated-use-of-weak should not warn about IBOutlet properties

Revision r211132 was supposed to disable -Warc-repeated-use-of-weak for
Objective-C properties marked with the IBOutlet attribute. Those properties
are supposed to be weak but they are only accessed from the main thread
so there is no risk of asynchronous updates setting them to nil. That
combination makes -Warc-repeated-use-of-weak very noisy. The previous
change only handled one kind of access to weak IBOutlet properties.
Instead of trying to add checks for all the different kinds of property
accesses, this patch removes the previous special case check and adds a
check at the point where the diagnostic is reported. rdar://21366461

Modified:
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/lib/Sema/SemaPseudoObject.cpp
cfe/trunk/test/SemaObjC/iboutlet.m

Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=270665=270664=270665=diff
==
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Wed May 25 00:41:57 2016
@@ -1334,6 +1334,12 @@ static void diagnoseRepeatedUseOfWeak(Se
 else
   llvm_unreachable("Unexpected weak object kind!");
 
+// Do not warn about IBOutlet weak property receivers being set to null
+// since they are typically only used from the main thread.
+if (const ObjCPropertyDecl *Prop = dyn_cast(D))
+  if (Prop->hasAttr())
+continue;
+
 // Show the first time the object was read.
 S.Diag(FirstRead->getLocStart(), DiagKind)
   << int(ObjectKind) << D << int(FunctionKind)

Modified: cfe/trunk/lib/Sema/SemaPseudoObject.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaPseudoObject.cpp?rev=270665=270664=270665=diff
==
--- cfe/trunk/lib/Sema/SemaPseudoObject.cpp (original)
+++ cfe/trunk/lib/Sema/SemaPseudoObject.cpp Wed May 25 00:41:57 2016
@@ -578,7 +578,7 @@ bool ObjCPropertyOpBuilder::isWeakProper
   if (RefExpr->isExplicitProperty()) {
 const ObjCPropertyDecl *Prop = RefExpr->getExplicitProperty();
 if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak)
-  return !Prop->hasAttr();
+  return true;
 
 T = Prop->getType();
   } else if (Getter) {

Modified: cfe/trunk/test/SemaObjC/iboutlet.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/iboutlet.m?rev=270665=270664=270665=diff
==
--- cfe/trunk/test/SemaObjC/iboutlet.m (original)
+++ cfe/trunk/test/SemaObjC/iboutlet.m Wed May 25 00:41:57 2016
@@ -41,6 +41,7 @@ IBInspectable @property (readonly) IBOut
 
 // rdar://15885642
 @interface WeakOutlet 
+@property int Number;
 @property IBOutlet __weak WeakOutlet* WeakProp;
 @end
 
@@ -50,3 +51,9 @@ WeakOutlet* func() {
   pwi.WeakProp = pwi.WeakProp;
   return pwi.WeakProp;
 }
+
+WeakOutlet* func2(WeakOutlet* pwi) {
+  [[pwi WeakProp] setNumber:0];
+  [[pwi WeakProp] setNumber:1];
+  return [pwi WeakProp];
+}


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r270666 - Rename a variable to avoid shadowing function parameter. NFC.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Wed May 25 00:42:00 2016
New Revision: 270666

URL: http://llvm.org/viewvc/llvm-project?rev=270666=rev
Log:
Rename a variable to avoid shadowing function parameter. NFC.

Modified:
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp

Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=270666=270665=270666=diff
==
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Wed May 25 00:42:00 2016
@@ -1322,27 +1322,27 @@ static void diagnoseRepeatedUseOfWeak(Se
   Ivar
 } ObjectKind;
 
-const NamedDecl *D = Key.getProperty();
-if (isa(D))
+const NamedDecl *KeyProp = Key.getProperty();
+if (isa(KeyProp))
   ObjectKind = Variable;
-else if (isa(D))
+else if (isa(KeyProp))
   ObjectKind = Property;
-else if (isa(D))
+else if (isa(KeyProp))
   ObjectKind = ImplicitProperty;
-else if (isa(D))
+else if (isa(KeyProp))
   ObjectKind = Ivar;
 else
   llvm_unreachable("Unexpected weak object kind!");
 
 // Do not warn about IBOutlet weak property receivers being set to null
 // since they are typically only used from the main thread.
-if (const ObjCPropertyDecl *Prop = dyn_cast(D))
+if (const ObjCPropertyDecl *Prop = dyn_cast(KeyProp))
   if (Prop->hasAttr())
 continue;
 
 // Show the first time the object was read.
 S.Diag(FirstRead->getLocStart(), DiagKind)
-  << int(ObjectKind) << D << int(FunctionKind)
+  << int(ObjectKind) << KeyProp << int(FunctionKind)
   << FirstRead->getSourceRange();
 
 // Print all the other accesses as notes.


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] arc-repeated-use-of-weak should not warn about IBOutlet properties

[Bob Wilson via cfe-commits]

> On May 24, 2016, at 11:21 AM, Manman  wrote:
> 
> 
>> On May 23, 2016, at 8:15 PM, Bob Wilson  wrote:
>> 
>> Revision r211132 was supposed to disable -Warc-repeated-use-of-weak for 
>> Objective-C properties marked with the IBOutlet attribute. Those properties 
>> are supposed to be weak but they are only accessed from the main thread so 
>> there is no risk of asynchronous updates setting them to nil. That 
>> combination makes -Warc-repeated-use-of-weak very noisy. The previous change 
>> only handled one kind of access to weak IBOutlet properties. Instead of 
>> trying to add checks for all the different kinds of property accesses, this 
>> patch removes the previous special case check and adds a check at the point 
>> where the diagnostic is reported.
> 
> The approach looks good to me in general. 
>> diff --git lib/Sema/AnalysisBasedWarnings.cpp 
>> lib/Sema/AnalysisBasedWarnings.cpp
>> index 3f2c41b..eb45315 100644
>> --- lib/Sema/AnalysisBasedWarnings.cpp
>> +++ lib/Sema/AnalysisBasedWarnings.cpp
>> @@ -1334,6 +1334,12 @@ static void diagnoseRepeatedUseOfWeak(Sema ,
>> else
>>   llvm_unreachable("Unexpected weak object kind!");
>> 
>> +// Do not warn about IBOutlet weak property receivers being set to null
>> +// since they are typically only used from the main thread.
>> +if (const ObjCPropertyDecl *Prop = dyn_cast(D))
>> +  if (Prop->hasAttr())
>> +continue;
>> +
>> // Show the first time the object was read.
>> S.Diag(FirstRead->getLocStart(), DiagKind)
>>   << int(ObjectKind) << D << int(FunctionKind)
> 
> Should we guard the call to diagnoseRepeatedUseOfWeak, instead of checking 
> the decl inside a loop in diagnoseRepeatedUseOfWeak?
> 
>  if (S.getLangOpts().ObjCWeak &&
>  !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, D->getLocStart()))
>diagnoseRepeatedUseOfWeak(S, fscope, D, AC.getParentMap());
> —> check IBOutlet here?

diagnoseRepeatedUseOfWeak is used to report all of the issues within a 
function. Some of those may involve IBOutlet properties and others not. We have 
to check each one separately.

Your comment prompted me to look more closely and I realized that the code is 
confusing because there is a local variable “D” that shadows the “const Decl 
*D” argument of diagnoseRepeatedUseOfWeak:

const NamedDecl *D = Key.getProperty();

We should rename that to avoid potential confusion. I can do that as a 
follow-up change.

> 
>> diff --git lib/Sema/SemaPseudoObject.cpp lib/Sema/SemaPseudoObject.cpp
>> index 62c823b..c93d800 100644
>> --- lib/Sema/SemaPseudoObject.cpp
>> +++ lib/Sema/SemaPseudoObject.cpp
>> @@ -578,7 +578,7 @@ bool ObjCPropertyOpBuilder::isWeakProperty() const {
>>   if (RefExpr->isExplicitProperty()) {
>> const ObjCPropertyDecl *Prop = RefExpr->getExplicitProperty();
>> if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak)
>> -  return !Prop->hasAttr();
>> +  return true;
>> 
>> T = Prop->getType();
>>   } else if (Getter) {
> 
> I wonder if this change is necessary to make the testing case pass, or is it 
> introduced for clarity, to better reflect the function name isWeakProperty?

This is the one special-case check that was introduced by r211132. I removed it 
because there is no need for it after we add the check in 
diagnoseRepeatedUseOfWeak.

> 
> Thanks,
> Manman
> 
>> rdar://problem/21366461
>> 
>> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] arc-repeated-use-of-weak should not warn about IBOutlet properties

[Bob Wilson via cfe-commits]

Revision r211132 was supposed to disable -Warc-repeated-use-of-weak for 
Objective-C properties marked with the IBOutlet attribute. Those properties are 
supposed to be weak but they are only accessed from the main thread so there is 
no risk of asynchronous updates setting them to nil. That combination makes 
-Warc-repeated-use-of-weak very noisy. The previous change only handled one 
kind of access to weak IBOutlet properties. Instead of trying to add checks for 
all the different kinds of property accesses, this patch removes the previous 
special case check and adds a check at the point where the diagnostic is 
reported. rdar://problem/21366461



clang.patch
Description: Binary data
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r270391 - Fix typo in documentation comment.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Sun May 22 20:52:50 2016
New Revision: 270391

URL: http://llvm.org/viewvc/llvm-project?rev=270391=rev
Log:
Fix typo in documentation comment.

Modified:
cfe/trunk/include/clang/Lex/ModuleMap.h

Modified: cfe/trunk/include/clang/Lex/ModuleMap.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/ModuleMap.h?rev=270391=270390=270391=diff
==
--- cfe/trunk/include/clang/Lex/ModuleMap.h (original)
+++ cfe/trunk/include/clang/Lex/ModuleMap.h Sun May 22 20:52:50 2016
@@ -59,7 +59,7 @@ public:
   /// \brief Called when an umbrella header is added during module map parsing.
   ///
   /// \param FileMgr FileManager instance
-  /// \param Header The umbreall header to collect.
+  /// \param Header The umbrella header to collect.
   virtual void moduleMapAddUmbrellaHeader(FileManager *FileMgr,
   const FileEntry *Header) {}
 };


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r266938 - Remove the (ignored) -Wreceived-is-weak diagnostic.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Wed Apr 20 19:11:24 2016
New Revision: 266938

URL: http://llvm.org/viewvc/llvm-project?rev=266938=rev
Log:
Remove the (ignored) -Wreceived-is-weak diagnostic.

We kept this around for a while since Xcode 6 and earlier had a build
setting for this warning. It was removed in Xcode 7 so there should be
no need for this warning now.

Modified:
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/test/SemaObjC/iboutlet.m

Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=266938=266937=266938=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Wed Apr 20 19:11:24 2016
@@ -286,8 +286,6 @@ def : DiagGroup<"overflow">;
 def ForwardClassReceiver : DiagGroup<"receiver-forward-class">;
 def MethodAccess : DiagGroup<"objc-method-access">;
 def ObjCReceiver : DiagGroup<"receiver-expr">;
-// FIXME: Remove this when Xcode removes the warning setting.
-def : DiagGroup<"receiver-is-weak">;
 def OperatorNewReturnsNull : DiagGroup<"new-returns-null">;
 def OverlengthStrings : DiagGroup<"overlength-strings">;
 def OverloadedVirtual : DiagGroup<"overloaded-virtual">;

Modified: cfe/trunk/test/SemaObjC/iboutlet.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/iboutlet.m?rev=266938=266937=266938=diff
==
--- cfe/trunk/test/SemaObjC/iboutlet.m (original)
+++ cfe/trunk/test/SemaObjC/iboutlet.m Wed Apr 20 19:11:24 2016
@@ -1,7 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fobjc-arc -Wno-objc-root-class 
-Wreceiver-is-weak -Warc-repeated-use-of-weak -fobjc-runtime-has-weak -verify %s
-// RUN: %clang_cc1 -x objective-c++ -fsyntax-only -fobjc-arc 
-Wno-objc-root-class -Wreceiver-is-weak -Warc-repeated-use-of-weak 
-fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-arc -Wno-objc-root-class 
-Warc-repeated-use-of-weak -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -x objective-c++ -fsyntax-only -fobjc-arc 
-Wno-objc-root-class -Warc-repeated-use-of-weak -fobjc-runtime-has-weak -verify 
%s
 // rdar://11448209
-// rdar://20259376
 
 #define READONLY readonly
 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r264519 - Check if a path is already absolute before trying to make it so.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Sat Mar 26 13:55:13 2016
New Revision: 264519

URL: http://llvm.org/viewvc/llvm-project?rev=264519=rev
Log:
Check if a path is already absolute before trying to make it so.

The FileSystem::makeAbsolute function has been calculating the current
working directory unconditionally, even when it is not needed. This calls
down to llvm::sys::fs::current_path, which is relatively expensive
because it stats two directories, regardless of whether those paths are
already in the stat cache. The net effect is that when using the
VFS, every stat during header search turns into three stats. With this
change, we get back to a single stat for absolute directory paths.

Modified:
cfe/trunk/lib/Basic/VirtualFileSystem.cpp

Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=264519=264518=264519=diff
==
--- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
+++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Sat Mar 26 13:55:13 2016
@@ -100,6 +100,9 @@ FileSystem::getBufferForFile(const llvm:
 }
 
 std::error_code FileSystem::makeAbsolute(SmallVectorImpl ) const {
+  if (llvm::sys::path::is_absolute(Path))
+return std::error_code();
+
   auto WorkingDir = getCurrentWorkingDirectory();
   if (!WorkingDir)
 return WorkingDir.getError();


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18088: Add a new warning to notify users of mismatched SDK and deployment target

[Bob Wilson via cfe-commits]

bob.wilson added a comment.

Yes, in its current usage this warning is only used for Apple's SDKs, but how 
does it help to put "apple" in the name of the diagnostic? Are you concerned 
about a name conflict with a similar diagnostic for non-Apple SDKs?


http://reviews.llvm.org/D18088



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18088: Add a new warning to notify users of mismatched SDK and deployment target

[Bob Wilson via cfe-commits]

bob.wilson added a comment.

This is a good idea overall. See comments.



Comment at: include/clang/Basic/DiagnosticDriverKinds.td:198
@@ -197,2 +197,3 @@
   InGroup>;
+def warn_incompatible_sdk : Warning<"using SDK for '%0' but deploying to 
'%1'">;
 def warn_debug_compression_unavailable : Warning<"cannot compress debug 
sections (zlib not installed)">,

Instead of the driver option, you should add this to the diagnostic: 
InGroup>


Comment at: include/clang/Driver/Options.td:1090
@@ -1089,1 +1089,3 @@
 def Wframe_larger_than_EQ : Joined<["-"], "Wframe-larger-than=">, 
Group, Flags<[DriverOption]>;
+def Wincompatible_sdk : Flag<["-"], "Wincompatible-sdk">, Group,
+  Flags<[DriverOption]>;

You should not need to add this option. See above.


Comment at: lib/Driver/ToolChains.cpp:333
@@ +332,3 @@
+StringRef Darwin::getPlatformFamily() const {
+  switch(TargetPlatform) {
+  case DarwinPlatformKind::MacOS:

Please add a space before the open paren.


Comment at: lib/Driver/ToolChains.cpp:742
@@ +741,3 @@
+
+  if(Args.hasArg(options::OPT_Wincompatible_sdk)) {
+if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {

Please add a space before the open paren.


Comment at: lib/Driver/ToolChains.cpp:750
@@ +749,3 @@
+StringRef SDK = isysroot.slice(BeginSDK + 5, EndSDK);
+if(!SDK.startswith(getPlatformFamily()))
+  getDriver().Diag(diag::warn_incompatible_sdk) << SDK

Please add a space before the open paren. You should run the patch through 
clang-format since you have several formatting issues.


http://reviews.llvm.org/D18088



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r263299 - Add fix-it for format-security warnings.

[Bob Wilson via cfe-commits]

I think r263584 does what you suggest. Let me know if not.

> On Mar 11, 2016, at 4:53 PM, Bob Wilson via cfe-commits 
> <cfe-commits@lists.llvm.org> wrote:
> 
> OK. I will do that.
> 
>> On Mar 11, 2016, at 4:15 PM, David Blaikie <dblai...@gmail.com 
>> <mailto:dblai...@gmail.com>> wrote:
>> 
>> 
>> 
>> On Fri, Mar 11, 2016 at 4:14 PM, Bob Wilson via cfe-commits 
>> <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote:
>> I’m not sure how to interpret that. It says: "Clang must recover from errors 
>> as if the fix-it had been applied.” I suppose that format-security could be 
>> an error if you’re building with -Werror, but I had been interpreting that 
>> to mean an error that would block further compilation. Can someone clarify 
>> this expectation?
>> 
>> I don’t think we can change -Wformat-security to be a note.
>> 
>> The suggestion isn't to change it to be a note, but to keep the warning but 
>> move the fixit to a note associated with the warning.
>> 
>> That way -fix-it doesn't automatically apply the fix-it, which avoids the 
>> issue of recovery as-if the fixit was applied.
>>  
>> It is an existing warning, and people expect us to match GCC on it as well.
>> 
>> 
>>> On Mar 11, 2016, at 4:03 PM, Nico Weber <tha...@chromium.org 
>>> <mailto:tha...@chromium.org>> wrote:
>>> 
>>> I think http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints 
>>> <http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints> says that if 
>>> a fixit is on a warning, then clang should process the code as if the fixit 
>>> had been applied. That's not the case here, so I think the fixit should be 
>>> on a note instead.
>>> 
>>> On Fri, Mar 11, 2016 at 4:55 PM, Bob Wilson via cfe-commits 
>>> <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote:
>>> Author: bwilson
>>> Date: Fri Mar 11 15:55:37 2016
>>> New Revision: 263299
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=263299=rev 
>>> <http://llvm.org/viewvc/llvm-project?rev=263299=rev>
>>> Log:
>>> Add fix-it for format-security warnings.
>>> 
>>> Added:
>>> cfe/trunk/test/SemaObjC/format-strings-objc-fixit.m
>>> Modified:
>>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>> cfe/trunk/test/Sema/format-strings-fixit.c
>>> 
>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> URL: 
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=263299=263298=263299=diff
>>>  
>>> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=263299=263298=263299=diff>
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Mar 11 15:55:37 2016
>>> @@ -3621,20 +3621,32 @@ bool Sema::CheckFormatArguments(ArrayRef
>>>// format is either NSString or CFString. This is a hack to prevent
>>>// diag when using the NSLocalizedString and CFCopyLocalizedString macros
>>>// which are usually used in place of NS and CF string literals.
>>> -  if (Type == FST_NSString &&
>>> -  SourceMgr.isInSystemMacro(Args[format_idx]->getLocStart()))
>>> +  SourceLocation FormatLoc = Args[format_idx]->getLocStart();
>>> +  if (Type == FST_NSString && SourceMgr.isInSystemMacro(FormatLoc))
>>>  return false;
>>> 
>>>// If there are no arguments specified, warn with -Wformat-security, 
>>> otherwise
>>>// warn only with -Wformat-nonliteral.
>>> -  if (Args.size() == firstDataArg)
>>> -Diag(Args[format_idx]->getLocStart(),
>>> - diag::warn_format_nonliteral_noargs)
>>> +  if (Args.size() == firstDataArg) {
>>> +const SemaDiagnosticBuilder  =
>>> +  Diag(FormatLoc, diag::warn_format_nonliteral_noargs);
>>> +switch (Type) {
>>> +default:
>>> +  D << OrigFormatExpr->getSourceRange();
>>> +  break;
>>> +case FST_Kprintf:
>>> +case FST_FreeBSDKPrintf:
>>> +case FST_Printf:
>>> +  D << FixItHint::CreateInsertion(FormatLoc, "\"%s\", ");
>>> +  break;
>>> +case FST_NSString:
>>> +  D << FixItHint::CreateInsertion(FormatLoc, "@\

r263584 - Move the fixit for -Wformat-security to a note.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Tue Mar 15 15:56:38 2016
New Revision: 263584

URL: http://llvm.org/viewvc/llvm-project?rev=263584=rev
Log:
Move the fixit for -Wformat-security to a note.

r263299 added a fixit for the -Wformat-security warning, but that runs
into complications with our guideline that error recovery should be done
as-if the fixit had been applied. Putting the fixit on a note avoids that.

Removed:
cfe/trunk/test/SemaObjC/format-strings-objc-fixit.m
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/format-strings-fixit.c
cfe/trunk/test/Sema/format-strings.c
cfe/trunk/test/SemaCXX/format-strings-0x.cpp
cfe/trunk/test/SemaCXX/format-strings.cpp
cfe/trunk/test/SemaObjC/format-strings-objc.m

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=263584=263583=263584=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Mar 15 15:56:38 
2016
@@ -7152,6 +7152,8 @@ def warn_scanf_scanlist_incomplete : War
 def note_format_string_defined : Note<"format string is defined here">;
 def note_format_fix_specifier : Note<"did you mean to use '%0'?">;
 def note_printf_c_str: Note<"did you mean to call the %0 method?">;
+def note_format_security_fixit: Note<
+  "treat the string as an argument to avoid this">;
 
 def warn_null_arg : Warning<
   "null passed to a callee that requires a non-null argument">,

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=263584=263583=263584=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue Mar 15 15:56:38 2016
@@ -3628,19 +3628,20 @@ bool Sema::CheckFormatArguments(ArrayRef
   // If there are no arguments specified, warn with -Wformat-security, 
otherwise
   // warn only with -Wformat-nonliteral.
   if (Args.size() == firstDataArg) {
-const SemaDiagnosticBuilder  =
-  Diag(FormatLoc, diag::warn_format_nonliteral_noargs);
+Diag(FormatLoc, diag::warn_format_nonliteral_noargs)
+  << OrigFormatExpr->getSourceRange();
 switch (Type) {
 default:
-  D << OrigFormatExpr->getSourceRange();
   break;
 case FST_Kprintf:
 case FST_FreeBSDKPrintf:
 case FST_Printf:
-  D << FixItHint::CreateInsertion(FormatLoc, "\"%s\", ");
+  Diag(FormatLoc, diag::note_format_security_fixit)
+<< FixItHint::CreateInsertion(FormatLoc, "\"%s\", ");
   break;
 case FST_NSString:
-  D << FixItHint::CreateInsertion(FormatLoc, "@\"%@\", ");
+  Diag(FormatLoc, diag::note_format_security_fixit)
+<< FixItHint::CreateInsertion(FormatLoc, "@\"%@\", ");
   break;
 }
   } else {

Modified: cfe/trunk/test/Sema/format-strings-fixit.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/format-strings-fixit.c?rev=263584=263583=263584=diff
==
--- cfe/trunk/test/Sema/format-strings-fixit.c (original)
+++ cfe/trunk/test/Sema/format-strings-fixit.c Tue Mar 15 15:56:38 2016
@@ -16,8 +16,6 @@ typedef __UINTMAX_TYPE__ uintmax_t;
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 typedef __WCHAR_TYPE__ wchar_t;
 
-extern const char *NonliteralString;
-
 void test() {
   // Basic types
   printf("%s", (int) 123);
@@ -96,9 +94,6 @@ void test() {
   printf("%G", (long double) 42);
   printf("%a", (long double) 42);
   printf("%A", (long double) 42);
-
-  // nonliteral format
-  printf(NonliteralString);
 }
 
 int scanf(char const *, ...);
@@ -223,7 +218,6 @@ void test2(int intSAParm[static 2]) {
 // CHECK: printf("%LG", (long double) 42);
 // CHECK: printf("%La", (long double) 42);
 // CHECK: printf("%LA", (long double) 42);
-// CHECK: printf("%s", NonliteralString);
 
 // CHECK: scanf("%99s", str);
 // CHECK: scanf("%s", vstr);

Modified: cfe/trunk/test/Sema/format-strings.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/format-strings.c?rev=263584=263583=263584=diff
==
--- cfe/trunk/test/Sema/format-strings.c (original)
+++ cfe/trunk/test/Sema/format-strings.c Tue Mar 15 15:56:38 2016
@@ -29,15 +29,22 @@ void check_string_literal( FILE* fp, con
   va_start(ap,buf);
 
   printf(s); // expected-warning {{format string is not a string literal}}
+  // expected-note@-1{{treat the string as an argument to avoid this}}
   vprintf(s,ap); // expected-warning {{format string is not a string literal}}
   fprintf(fp,s); // expected-warning {{format string is not a string literal}}
+  // expected-note@-1{{treat the string as an 

Re: r263299 - Add fix-it for format-security warnings.

[Bob Wilson via cfe-commits]

OK. I will do that.

> On Mar 11, 2016, at 4:15 PM, David Blaikie <dblai...@gmail.com> wrote:
> 
> 
> 
> On Fri, Mar 11, 2016 at 4:14 PM, Bob Wilson via cfe-commits 
> <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote:
> I’m not sure how to interpret that. It says: "Clang must recover from errors 
> as if the fix-it had been applied.” I suppose that format-security could be 
> an error if you’re building with -Werror, but I had been interpreting that to 
> mean an error that would block further compilation. Can someone clarify this 
> expectation?
> 
> I don’t think we can change -Wformat-security to be a note.
> 
> The suggestion isn't to change it to be a note, but to keep the warning but 
> move the fixit to a note associated with the warning.
> 
> That way -fix-it doesn't automatically apply the fix-it, which avoids the 
> issue of recovery as-if the fixit was applied.
>  
> It is an existing warning, and people expect us to match GCC on it as well.
> 
> 
>> On Mar 11, 2016, at 4:03 PM, Nico Weber <tha...@chromium.org 
>> <mailto:tha...@chromium.org>> wrote:
>> 
>> I think http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints 
>> <http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints> says that if 
>> a fixit is on a warning, then clang should process the code as if the fixit 
>> had been applied. That's not the case here, so I think the fixit should be 
>> on a note instead.
>> 
>> On Fri, Mar 11, 2016 at 4:55 PM, Bob Wilson via cfe-commits 
>> <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote:
>> Author: bwilson
>> Date: Fri Mar 11 15:55:37 2016
>> New Revision: 263299
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=263299=rev 
>> <http://llvm.org/viewvc/llvm-project?rev=263299=rev>
>> Log:
>> Add fix-it for format-security warnings.
>> 
>> Added:
>> cfe/trunk/test/SemaObjC/format-strings-objc-fixit.m
>> Modified:
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>> cfe/trunk/test/Sema/format-strings-fixit.c
>> 
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=263299=263298=263299=diff
>>  
>> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=263299=263298=263299=diff>
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Mar 11 15:55:37 2016
>> @@ -3621,20 +3621,32 @@ bool Sema::CheckFormatArguments(ArrayRef
>>// format is either NSString or CFString. This is a hack to prevent
>>// diag when using the NSLocalizedString and CFCopyLocalizedString macros
>>// which are usually used in place of NS and CF string literals.
>> -  if (Type == FST_NSString &&
>> -  SourceMgr.isInSystemMacro(Args[format_idx]->getLocStart()))
>> +  SourceLocation FormatLoc = Args[format_idx]->getLocStart();
>> +  if (Type == FST_NSString && SourceMgr.isInSystemMacro(FormatLoc))
>>  return false;
>> 
>>// If there are no arguments specified, warn with -Wformat-security, 
>> otherwise
>>// warn only with -Wformat-nonliteral.
>> -  if (Args.size() == firstDataArg)
>> -Diag(Args[format_idx]->getLocStart(),
>> - diag::warn_format_nonliteral_noargs)
>> +  if (Args.size() == firstDataArg) {
>> +const SemaDiagnosticBuilder  =
>> +  Diag(FormatLoc, diag::warn_format_nonliteral_noargs);
>> +switch (Type) {
>> +default:
>> +  D << OrigFormatExpr->getSourceRange();
>> +  break;
>> +case FST_Kprintf:
>> +case FST_FreeBSDKPrintf:
>> +case FST_Printf:
>> +  D << FixItHint::CreateInsertion(FormatLoc, "\"%s\", ");
>> +  break;
>> +case FST_NSString:
>> +  D << FixItHint::CreateInsertion(FormatLoc, "@\"%@\", ");
>> +  break;
>> +}
>> +  } else {
>> +Diag(FormatLoc, diag::warn_format_nonliteral)
>><< OrigFormatExpr->getSourceRange();
>> -  else
>> -Diag(Args[format_idx]->getLocStart(),
>> - diag::warn_format_nonliteral)
>> -   << OrigFormatExpr->getSourceRange();
>> +  }
>>return false;
>>  }
>> 
>> 
>> Modified: cfe/trunk/test/Sema/format-strings-fixit.c
>> URL: 
>> http://llvm.org/viewvc/llvm-pro

Re: r263299 - Add fix-it for format-security warnings.

[Bob Wilson via cfe-commits]

I’m not sure how to interpret that. It says: "Clang must recover from errors as 
if the fix-it had been applied.” I suppose that format-security could be an 
error if you’re building with -Werror, but I had been interpreting that to mean 
an error that would block further compilation. Can someone clarify this 
expectation?

I don’t think we can change -Wformat-security to be a note. It is an existing 
warning, and people expect us to match GCC on it as well.

> On Mar 11, 2016, at 4:03 PM, Nico Weber <tha...@chromium.org> wrote:
> 
> I think http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints 
> <http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints> says that if a 
> fixit is on a warning, then clang should process the code as if the fixit had 
> been applied. That's not the case here, so I think the fixit should be on a 
> note instead.
> 
> On Fri, Mar 11, 2016 at 4:55 PM, Bob Wilson via cfe-commits 
> <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: bwilson
> Date: Fri Mar 11 15:55:37 2016
> New Revision: 263299
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=263299=rev 
> <http://llvm.org/viewvc/llvm-project?rev=263299=rev>
> Log:
> Add fix-it for format-security warnings.
> 
> Added:
> cfe/trunk/test/SemaObjC/format-strings-objc-fixit.m
> Modified:
> cfe/trunk/lib/Sema/SemaChecking.cpp
> cfe/trunk/test/Sema/format-strings-fixit.c
> 
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=263299=263298=263299=diff
>  
> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=263299=263298=263299=diff>
> ==
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Mar 11 15:55:37 2016
> @@ -3621,20 +3621,32 @@ bool Sema::CheckFormatArguments(ArrayRef
>// format is either NSString or CFString. This is a hack to prevent
>// diag when using the NSLocalizedString and CFCopyLocalizedString macros
>// which are usually used in place of NS and CF string literals.
> -  if (Type == FST_NSString &&
> -  SourceMgr.isInSystemMacro(Args[format_idx]->getLocStart()))
> +  SourceLocation FormatLoc = Args[format_idx]->getLocStart();
> +  if (Type == FST_NSString && SourceMgr.isInSystemMacro(FormatLoc))
>  return false;
> 
>// If there are no arguments specified, warn with -Wformat-security, 
> otherwise
>// warn only with -Wformat-nonliteral.
> -  if (Args.size() == firstDataArg)
> -Diag(Args[format_idx]->getLocStart(),
> - diag::warn_format_nonliteral_noargs)
> +  if (Args.size() == firstDataArg) {
> +const SemaDiagnosticBuilder  =
> +  Diag(FormatLoc, diag::warn_format_nonliteral_noargs);
> +switch (Type) {
> +default:
> +  D << OrigFormatExpr->getSourceRange();
> +  break;
> +case FST_Kprintf:
> +case FST_FreeBSDKPrintf:
> +case FST_Printf:
> +  D << FixItHint::CreateInsertion(FormatLoc, "\"%s\", ");
> +  break;
> +case FST_NSString:
> +  D << FixItHint::CreateInsertion(FormatLoc, "@\"%@\", ");
> +  break;
> +}
> +  } else {
> +Diag(FormatLoc, diag::warn_format_nonliteral)
><< OrigFormatExpr->getSourceRange();
> -  else
> -Diag(Args[format_idx]->getLocStart(),
> - diag::warn_format_nonliteral)
> -   << OrigFormatExpr->getSourceRange();
> +  }
>return false;
>  }
> 
> 
> Modified: cfe/trunk/test/Sema/format-strings-fixit.c
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/format-strings-fixit.c?rev=263299=263298=263299=diff
>  
> <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/format-strings-fixit.c?rev=263299=263298=263299=diff>
> ==
> --- cfe/trunk/test/Sema/format-strings-fixit.c (original)
> +++ cfe/trunk/test/Sema/format-strings-fixit.c Fri Mar 11 15:55:37 2016
> @@ -16,6 +16,8 @@ typedef __UINTMAX_TYPE__ uintmax_t;
>  typedef __PTRDIFF_TYPE__ ptrdiff_t;
>  typedef __WCHAR_TYPE__ wchar_t;
> 
> +extern const char *NonliteralString;
> +
>  void test() {
>// Basic types
>printf("%s", (int) 123);
> @@ -94,6 +96,9 @@ void test() {
>printf("%G", (long double) 42);
>printf("%a", (long double) 42);
>printf("%A", (long double) 42);
> +
> +  // nonliteral format
> +  printf(NonliteralString);
>  }

r263299 - Add fix-it for format-security warnings.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Fri Mar 11 15:55:37 2016
New Revision: 263299

URL: http://llvm.org/viewvc/llvm-project?rev=263299=rev
Log:
Add fix-it for format-security warnings.

Added:
cfe/trunk/test/SemaObjC/format-strings-objc-fixit.m
Modified:
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/format-strings-fixit.c

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=263299=263298=263299=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Mar 11 15:55:37 2016
@@ -3621,20 +3621,32 @@ bool Sema::CheckFormatArguments(ArrayRef
   // format is either NSString or CFString. This is a hack to prevent
   // diag when using the NSLocalizedString and CFCopyLocalizedString macros
   // which are usually used in place of NS and CF string literals.
-  if (Type == FST_NSString &&
-  SourceMgr.isInSystemMacro(Args[format_idx]->getLocStart()))
+  SourceLocation FormatLoc = Args[format_idx]->getLocStart();
+  if (Type == FST_NSString && SourceMgr.isInSystemMacro(FormatLoc))
 return false;
 
   // If there are no arguments specified, warn with -Wformat-security, 
otherwise
   // warn only with -Wformat-nonliteral.
-  if (Args.size() == firstDataArg)
-Diag(Args[format_idx]->getLocStart(),
- diag::warn_format_nonliteral_noargs)
+  if (Args.size() == firstDataArg) {
+const SemaDiagnosticBuilder  =
+  Diag(FormatLoc, diag::warn_format_nonliteral_noargs);
+switch (Type) {
+default:
+  D << OrigFormatExpr->getSourceRange();
+  break;
+case FST_Kprintf:
+case FST_FreeBSDKPrintf:
+case FST_Printf:
+  D << FixItHint::CreateInsertion(FormatLoc, "\"%s\", ");
+  break;
+case FST_NSString:
+  D << FixItHint::CreateInsertion(FormatLoc, "@\"%@\", ");
+  break;
+}
+  } else {
+Diag(FormatLoc, diag::warn_format_nonliteral)
   << OrigFormatExpr->getSourceRange();
-  else
-Diag(Args[format_idx]->getLocStart(),
- diag::warn_format_nonliteral)
-   << OrigFormatExpr->getSourceRange();
+  }
   return false;
 }
 

Modified: cfe/trunk/test/Sema/format-strings-fixit.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/format-strings-fixit.c?rev=263299=263298=263299=diff
==
--- cfe/trunk/test/Sema/format-strings-fixit.c (original)
+++ cfe/trunk/test/Sema/format-strings-fixit.c Fri Mar 11 15:55:37 2016
@@ -16,6 +16,8 @@ typedef __UINTMAX_TYPE__ uintmax_t;
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 typedef __WCHAR_TYPE__ wchar_t;
 
+extern const char *NonliteralString;
+
 void test() {
   // Basic types
   printf("%s", (int) 123);
@@ -94,6 +96,9 @@ void test() {
   printf("%G", (long double) 42);
   printf("%a", (long double) 42);
   printf("%A", (long double) 42);
+
+  // nonliteral format
+  printf(NonliteralString);
 }
 
 int scanf(char const *, ...);
@@ -218,6 +223,7 @@ void test2(int intSAParm[static 2]) {
 // CHECK: printf("%LG", (long double) 42);
 // CHECK: printf("%La", (long double) 42);
 // CHECK: printf("%LA", (long double) 42);
+// CHECK: printf("%s", NonliteralString);
 
 // CHECK: scanf("%99s", str);
 // CHECK: scanf("%s", vstr);

Added: cfe/trunk/test/SemaObjC/format-strings-objc-fixit.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/format-strings-objc-fixit.m?rev=263299=auto
==
--- cfe/trunk/test/SemaObjC/format-strings-objc-fixit.m (added)
+++ cfe/trunk/test/SemaObjC/format-strings-objc-fixit.m Fri Mar 11 15:55:37 2016
@@ -0,0 +1,31 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x objective-c -triple x86_64-apple-darwin 
-Wno-objc-root-class -pedantic -Wall -fixit %t
+// RUN: %clang_cc1 -x objective-c -triple x86_64-apple-darwin 
-Wno-objc-root-class -fsyntax-only -pedantic -Wall -Werror %t
+// RUN: %clang_cc1 -x objective-c -triple x86_64-apple-darwin 
-Wno-objc-root-class -E -o - %t | FileCheck %s
+
+typedef signed char BOOL;
+typedef unsigned int NSUInteger;
+typedef struct _NSZone NSZone;
+@class NSCoder, NSString, NSEnumerator;
+@protocol NSObject  - (BOOL)isEqual:(id)object; @end
+@protocol NSCopying  - (id)copyWithZone:(NSZone *)zone; @end
+@protocol NSMutableCopying  - (id)mutableCopyWithZone:(NSZone *)zone; @end
+@protocol NSCoding  - (void)encodeWithCoder:(NSCoder *)aCoder; @end
+@interface NSObject  {} @end
+@interface NSString : NSObject   - 
(NSUInteger)length; @end
+extern void NSLog(NSString *format, ...);
+
+/* This is a test of the various code modification hints that are
+   provided as part of warning or extension diagnostics. All of the
+   warnings will be fixed by -fixit, and the resulting file should
+   compile cleanly with -Werror -pedantic. */
+
+extern NSString *NonliteralString;

Re: [PATCH] D17941: add fix-its for format-security warnings

[Bob Wilson via cfe-commits]

bob.wilson accepted this revision.
bob.wilson added a reviewer: bob.wilson.
bob.wilson added a comment.
This revision is now accepted and ready to land.

Thanks Ben. Committed in r263299


http://reviews.llvm.org/D17941



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17865: Add replacement = "xxx" to DeprecatedAttr.

[Bob Wilson via cfe-commits]

bob.wilson added a comment.

> I do like the explicit nature of this approach, but I'm worried about it 
> being too novel. For instance, would this compel GCC to implement a similar 
> parsing feature since they also support the deprecated attribute, that sort 
> of thing. Also, it feels like a bit of an odd design in C and C++ since 
> nothing else in the language supports argument passing by name like that (and 
> I would really hate to see us use = blah only to find out that C++ someday 
> standardizes on something different).


This makes sense to me. We also want to add this to the "availability" 
attribute, and since that already uses named arguments, I think we should use 
the "replacement=" syntax there.


http://reviews.llvm.org/D17865



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17941: add fix-its for format-security warnings

[Bob Wilson via cfe-commits]

bob.wilson added a comment.

In http://reviews.llvm.org/D17941#369698, @bcraig wrote:

> What about wprintf?  Do we currently warn for wprintf(str)?  If so, then the 
> fixit probably needs to involve L"%ls".


Darwin does not mark wprintf functions with an attribute. Linux (at least the 
version I checked) has an attribute that is commented out, but it uses a 
distinct "__wprintf__" format type. Clang does not currently support that 
format type. If that is added in the future, you are right that the fix-it will 
need to be different.


http://reviews.llvm.org/D17941



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D17941: add fix-its for format-security warnings

[Bob Wilson via cfe-commits]

bob.wilson created this revision.
bob.wilson added reviewers: bcraig, rjmccall, dblaikie.
bob.wilson added a subscriber: cfe-commits.
Herald added a subscriber: mcrosier.

The format-security warning is a special case of format-nonliteral that applies 
when there are no arguments besides the format string. In those cases, for 
printf and NSLog-style functions, there is an easy fix to provide a literal 
format string of "%s" (or @"%@" for Objective-C), with the nonliteral string as 
the argument. This patch teaches clang to provide fix-its for those cases.

http://reviews.llvm.org/D17941

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/format-strings-fixit.c
  test/SemaObjC/format-strings-objc-fixit.m

Index: test/SemaObjC/format-strings-objc-fixit.m
===
--- /dev/null
+++ test/SemaObjC/format-strings-objc-fixit.m
@@ -0,0 +1,31 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x objective-c -triple x86_64-apple-darwin -Wno-objc-root-class -pedantic -Wall -fixit %t
+// RUN: %clang_cc1 -x objective-c -triple x86_64-apple-darwin -Wno-objc-root-class -fsyntax-only -pedantic -Wall -Werror %t
+// RUN: %clang_cc1 -x objective-c -triple x86_64-apple-darwin -Wno-objc-root-class -E -o - %t | FileCheck %s
+
+typedef signed char BOOL;
+typedef unsigned int NSUInteger;
+typedef struct _NSZone NSZone;
+@class NSCoder, NSString, NSEnumerator;
+@protocol NSObject  - (BOOL)isEqual:(id)object; @end
+@protocol NSCopying  - (id)copyWithZone:(NSZone *)zone; @end
+@protocol NSMutableCopying  - (id)mutableCopyWithZone:(NSZone *)zone; @end
+@protocol NSCoding  - (void)encodeWithCoder:(NSCoder *)aCoder; @end
+@interface NSObject  {} @end
+@interface NSString : NSObject   - (NSUInteger)length; @end
+extern void NSLog(NSString *format, ...);
+
+/* This is a test of the various code modification hints that are
+   provided as part of warning or extension diagnostics. All of the
+   warnings will be fixed by -fixit, and the resulting file should
+   compile cleanly with -Werror -pedantic. */
+
+extern NSString *NonliteralString;
+
+void test() {
+  // nonliteral format
+  NSLog(NonliteralString);
+}
+
+// Validate the fixes.
+// CHECK: NSLog(@"%@", NonliteralString);
Index: test/Sema/format-strings-fixit.c
===
--- test/Sema/format-strings-fixit.c
+++ test/Sema/format-strings-fixit.c
@@ -16,6 +16,8 @@
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 typedef __WCHAR_TYPE__ wchar_t;
 
+extern const char *NonliteralString;
+
 void test() {
   // Basic types
   printf("%s", (int) 123);
@@ -94,6 +96,9 @@
   printf("%G", (long double) 42);
   printf("%a", (long double) 42);
   printf("%A", (long double) 42);
+
+  // nonliteral format
+  printf(NonliteralString);
 }
 
 int scanf(char const *, ...);
@@ -218,6 +223,7 @@
 // CHECK: printf("%LG", (long double) 42);
 // CHECK: printf("%La", (long double) 42);
 // CHECK: printf("%LA", (long double) 42);
+// CHECK: printf("%s", NonliteralString);
 
 // CHECK: scanf("%99s", str);
 // CHECK: scanf("%s", vstr);
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3621,20 +3621,32 @@
   // format is either NSString or CFString. This is a hack to prevent
   // diag when using the NSLocalizedString and CFCopyLocalizedString macros
   // which are usually used in place of NS and CF string literals.
-  if (Type == FST_NSString &&
-  SourceMgr.isInSystemMacro(Args[format_idx]->getLocStart()))
+  SourceLocation FormatLoc = Args[format_idx]->getLocStart();
+  if (Type == FST_NSString && SourceMgr.isInSystemMacro(FormatLoc))
 return false;
 
   // If there are no arguments specified, warn with -Wformat-security, otherwise
   // warn only with -Wformat-nonliteral.
-  if (Args.size() == firstDataArg)
-Diag(Args[format_idx]->getLocStart(),
- diag::warn_format_nonliteral_noargs)
+  if (Args.size() == firstDataArg) {
+const SemaDiagnosticBuilder  =
+  Diag(FormatLoc, diag::warn_format_nonliteral_noargs);
+switch (Type) {
+default:
+  D << OrigFormatExpr->getSourceRange();
+  break;
+case FST_Kprintf:
+case FST_FreeBSDKPrintf:
+case FST_Printf:
+  D << FixItHint::CreateInsertion(FormatLoc, "\"%s\", ");
+  break;
+case FST_NSString:
+  D << FixItHint::CreateInsertion(FormatLoc, "@\"%@\", ");
+  break;
+}
+  } else {
+Diag(FormatLoc, diag::warn_format_nonliteral)
   << OrigFormatExpr->getSourceRange();
-  else
-Diag(Args[format_idx]->getLocStart(),
- diag::warn_format_nonliteral)
-   << OrigFormatExpr->getSourceRange();
+  }
   return false;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r261762 - Fix typo in test/CodeGen/object-size.c CHECK line.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Wed Feb 24 12:38:35 2016
New Revision: 261762

URL: http://llvm.org/viewvc/llvm-project?rev=261762=rev
Log:
Fix typo in test/CodeGen/object-size.c CHECK line.

Modified:
cfe/trunk/test/CodeGen/object-size.c

Modified: cfe/trunk/test/CodeGen/object-size.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/object-size.c?rev=261762=261761=261762=diff
==
--- cfe/trunk/test/CodeGen/object-size.c (original)
+++ cfe/trunk/test/CodeGen/object-size.c Wed Feb 24 12:38:35 2016
@@ -505,7 +505,7 @@ void test31() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
   gi = __builtin_object_size(ds1[9].snd, 1);
 
-  // CHECH: store i32 2
+  // CHECK: store i32 2
   gi = __builtin_object_size([9].snd[0], 1);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r260787 - [Sema] More changes to fix Objective-C fallout from r249995.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Fri Feb 12 19:41:41 2016
New Revision: 260787

URL: http://llvm.org/viewvc/llvm-project?rev=260787=rev
Log:
[Sema] More changes to fix Objective-C fallout from r249995.

This is a follow-up to PR26085. That was fixed in r257710 but the testcase
there was incomplete. There is a related issue where the overload resolution
for Objective-C incorrectly picks a method that is not valid without a
bridge cast. The call to Sema::CheckSingleAssignmentConstraints that was
added to SemaOverload.cpp's IsStandardConversion() function does not catch
that case and reports that the method is Compatible even when it is not.

The root cause here is that various Objective-C-related functions in Sema
do not consistently return a value to indicate whether there was an error.
This was fine in the past because they would report diagnostics when needed,
but r257710 changed them to suppress reporting diagnostics when checking
during overload resolution.

This patch adds a new ACR_error result to the ARCConversionResult enum and
updates Sema::CheckObjCARCConversion to return that value when there is an
error. Most of the calls to that function do not check the return value,
so adding this new result does not affect them. The one exception is in
SemaCast.cpp where it specifically checks for ACR_unbridged, so that is
also OK. The call in Sema::CheckSingleAssignmentConstraints can then check
for an ACR_okay result and identify assignments as Incompatible. To
preserve the existing behavior, it only changes the return value to
Incompatible when the new Diagnose argument (from r257710) is false.

Similarly, the CheckObjCBridgeRelatedConversions and
ConversionToObjCStringLiteralCheck need to identify when an assignment is
Incompatible. Those functions already return appropriate values but they
need some fixes related to the new Diagnose argument.

Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/lib/Sema/SemaExprObjC.cpp
cfe/trunk/test/SemaObjC/ovl-check.m

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=260787=260786=260787=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Fri Feb 12 19:41:41 2016
@@ -8654,7 +8654,7 @@ public:
 Expr *CastExpr,
 SourceLocation RParenLoc);
 
-  enum ARCConversionResult { ACR_okay, ACR_unbridged };
+  enum ARCConversionResult { ACR_okay, ACR_unbridged, ACR_error };
 
   /// \brief Checks for invalid conversions and casts between
   /// retainable pointers and other pointer kinds.

Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=260787=260786=260787=diff
==
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Feb 12 19:41:41 2016
@@ -7566,13 +7566,24 @@ Sema::CheckSingleAssignmentConstraints(Q
   if (result != Incompatible && RHS.get()->getType() != LHSType) {
 QualType Ty = LHSType.getNonLValueExprType(Context);
 Expr *E = RHS.get();
-if (getLangOpts().ObjCAutoRefCount)
-  CheckObjCARCConversion(SourceRange(), Ty, E, CCK_ImplicitConversion,
- Diagnose, DiagnoseCFAudited);
+
+// Check for various Objective-C errors. If we are not reporting
+// diagnostics and just checking for errors, e.g., during overload
+// resolution, return Incompatible to indicate the failure.
+if (getLangOpts().ObjCAutoRefCount &&
+CheckObjCARCConversion(SourceRange(), Ty, E, CCK_ImplicitConversion,
+   Diagnose, DiagnoseCFAudited) != ACR_okay) {
+  if (!Diagnose)
+return Incompatible;
+}
 if (getLangOpts().ObjC1 &&
 (CheckObjCBridgeRelatedConversions(E->getLocStart(), LHSType,
E->getType(), E, Diagnose) ||
  ConversionToObjCStringLiteralCheck(LHSType, E, Diagnose))) {
+  if (!Diagnose)
+return Incompatible;
+  // Replace the expression with a corrected version and continue so we
+  // can find further errors.
   RHS = E;
   return Compatible;
 }
@@ -12035,10 +12046,11 @@ bool Sema::ConversionToObjCStringLiteral
   StringLiteral *SL = dyn_cast(SrcExpr);
   if (!SL || !SL->isAscii())
 return false;
-  if (Diagnose)
+  if (Diagnose) {
 Diag(SL->getLocStart(), diag::err_missing_atsign_prefix)
   << FixItHint::CreateInsertion(SL->getLocStart(), "@");
-  Exp = BuildObjCStringLiteral(SL->getLocStart(), SL).get();
+Exp = BuildObjCStringLiteral(SL->getLocStart(), SL).get();
+  }
   return true;
 }
 

Modified: cfe/trunk/lib/Sema/SemaExprObjC.cpp
URL: 

Re: [PATCH] D17166: [Sema] More changes to fix Objective-C fallout from r249995.

[Bob Wilson via cfe-commits]

bob.wilson closed this revision.
bob.wilson marked an inline comment as done.
bob.wilson added a comment.

Committed in r260787.

I had previously missed a regression in one of the existing ovl-check.m tests. 
The testTakesCFTypeRef function was checking that the CFTypeRef overload was 
selected. However, that does not match the behavior prior to r249995, where 
neither overload candidate is chosen as the "best match" and clang falls back 
to using the first one. With my change here, the previous behavior is restored. 
I reordered the two overload candidates in the test to match that. While I was 
at it, I also merged my "Type2" typedef with the existing CFTypeRef in the test.



Comment at: lib/Sema/SemaExpr.cpp:7580
@@ +7579,3 @@
+  if (Diagnose) {
+// If an error was diagnosed here, replace the expression with a
+// corrected version and continue so we can find further errors.

george.burgess.iv wrote:
> Because we're doing a similar thing at line 7573 (not necessarily replacing 
> an expression, but still answering differently depending on the value of 
> `Diagnose`), can we either put a similar comment there, or make this comment 
> a bit more general and move it above `if (getLangOpts().ObjCAutoRefCount && 
> [...]`?
Good idea. I'm going to put a comment above the "if" but I'll also leave 
something to explain why the expression is replaced. (I'm also going to change 
those "result = Incompatible" lines to just return, since there is no point in 
continuing with further checks if we're not reporting the diagnostics anyway.)


http://reviews.llvm.org/D17166



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D17166: [Sema] More changes to fix Objective-C fallout from r249995.

[Bob Wilson via cfe-commits]

bob.wilson created this revision.
bob.wilson added reviewers: cfe-commits, george.burgess.iv, doug.gregor, rsmith.
Herald added a subscriber: mcrosier.

This is a follow-up to PR26085. That was fixed in r257710 but the testcase 
there was incomplete. There is a related issue where the overload resolution 
for Objective-C incorrectly picks a method that is not valid without a bridge 
cast. The call to Sema::CheckSingleAssignmentConstraints that was added to 
SemaOverload.cpp's IsStandardConversion() function does not catch that case and 
reports that the method is Compatible even when it is not.

The root cause here is that various Objective-C-related functions in Sema do 
not consistently return a value to indicate whether there was an error. This 
was fine in the past because they would report diagnostics when needed, but 
r257710 changed them to suppress reporting diagnostics when checking during 
overload resolution.

This patch adds a new ACR_error result to the ARCConversionResult enum and 
updates Sema::CheckObjCARCConversion to return that value when there is an 
error. Most of the calls to that function do not check the return value, so 
adding this new result does not affect them. The one exception is in 
SemaCast.cpp where it specifically checks for ACR_unbridged, so that is also 
OK. The call in Sema::CheckSingleAssignmentConstraints can then check for an 
ACR_okay result and identify assignments as Incompatible. To preserve the 
existing behavior, it only changes the return value to Incompatible when the 
new Diagnose argument (from r257710) is false.

Similarly, the CheckObjCBridgeRelatedConversions and 
ConversionToObjCStringLiteralCheck need to identify when an assignment is 
Incompatible. Those functions already return appropriate values but they need 
some fixes related to the new Diagnose argument.

http://reviews.llvm.org/D17166

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprObjC.cpp
  test/SemaObjC/ovl-check.m

Index: test/SemaObjC/ovl-check.m
===
--- test/SemaObjC/ovl-check.m
+++ test/SemaObjC/ovl-check.m
@@ -4,20 +4,24 @@
 // in overload resolution in ObjC.
 
 struct Type1 { int a; };
+typedef const __attribute__((objc_bridge(id))) void * Type2;
 @interface Iface1 @end
 
 @interface NeverCalled
 - (void) test:(struct Type1 *)arg;
+- (void) test2:(Type2 )arg;
 @end
 
 @interface TakesIface1
 - (void) test:(Iface1 *)arg;
+- (void) test2:(Iface1 *)arg;
 @end
 
 // PR26085, rdar://problem/24111333
 void testTakesIface1(id x, Iface1 *arg) {
   // This should resolve silently to `TakesIface1`.
   [x test:arg];
+  [x test2:arg];
 }
 
 @class NSString;
Index: lib/Sema/SemaExprObjC.cpp
===
--- lib/Sema/SemaExprObjC.cpp
+++ lib/Sema/SemaExprObjC.cpp
@@ -3478,6 +3478,8 @@
 return;
 
   QualType castExprType = castExpr->getType();
+  // Defer emitting a diagnostic for bridge-related casts; that will be
+  // handled by CheckObjCBridgeRelatedConversions.
   TypedefNameDecl *TDNDecl = nullptr;
   if ((castACTC == ACTC_coreFoundation &&  exprACTC == ACTC_retainable &&
ObjCBridgeRelatedAttrFromType(castType, TDNDecl)) ||
@@ -3922,16 +3924,16 @@
   << FixItHint::CreateInsertion(SrcExprEndLoc, "]");
 Diag(RelatedClass->getLocStart(), diag::note_declared_at);
 Diag(TDNDecl->getLocStart(), diag::note_declared_at);
-  }
   
-  QualType receiverType = Context.getObjCInterfaceType(RelatedClass);
-  // Argument.
-  Expr *args[] = { SrcExpr };
-  ExprResult msg = BuildClassMessageImplicit(receiverType, false,
+QualType receiverType = Context.getObjCInterfaceType(RelatedClass);
+// Argument.
+Expr *args[] = { SrcExpr };
+ExprResult msg = BuildClassMessageImplicit(receiverType, false,
   ClassMethod->getLocation(),
   ClassMethod->getSelector(), ClassMethod,
   MultiExprArg(args, 1));
-  SrcExpr = msg.get();
+SrcExpr = msg.get();
+  }
   return true;
 }
   }
@@ -3965,14 +3967,14 @@
 }
 Diag(RelatedClass->getLocStart(), diag::note_declared_at);
 Diag(TDNDecl->getLocStart(), diag::note_declared_at);
-  }
   
-  ExprResult msg =
-BuildInstanceMessageImplicit(SrcExpr, SrcType,
- InstanceMethod->getLocation(),
- InstanceMethod->getSelector(),
- InstanceMethod, None);
-  SrcExpr = msg.get();
+ExprResult msg =
+  BuildInstanceMessageImplicit(SrcExpr, SrcType,
+   InstanceMethod->getLocation(),
+   InstanceMethod->getSelector(),
+   InstanceMethod, None);
+   

Re: [libcxx] r260012 - Cleanup node-type handling in the unordered containers

[Bob Wilson via cfe-commits]

> On Feb 10, 2016, at 12:59 PM, Tim Northover  wrote:
> 
> On 10 February 2016 at 12:52, Eric Fiselier  wrote:
>> @Tim Are these tests in the clang test suite?
> 
> Yep, at http://llvm.org/git/test-suite.git.
> 
>> Marshall and I were just talking about removing  all together. 
>> Could you explain who still uses it?
> 
> In the test-suite, it looks like it's just these two micro-benchmarks.
> I doubt anyone would complain about converting them to use
> std::unordered_map (especially since the underlying implementation
> seems to be shared anyway).
> 
> Outside that, we seem to have a handful of internal users and Bob may
> have ABI backwards-compatibility concerns (or not). I've added him
> explicitly.

I will defer to Duncan on that question.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Bob Wilson via cfe-commits]

> On Feb 3, 2016, at 12:23 PM, Xinliang David Li  wrote:
> 
> On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson  > wrote:
>> 
>>> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits 
>>>  wrote:
>>> 
>>> silvas added a comment.
>>> 
>>> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
>>> 
 For the longer term, one possible solution is to make FE based
 instrumentation only used for coverage testing which can be turned on
 with -fcoverage-mapping option (currently, -fcoverage-mapping can not
 be used alone and must be used together with
 -fprofile-instr-generate). To summarize:
 
 A. Current behavior:
 
 ---
 
 1. -fprofile-instr-generate turns on FE based instrumentation
 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based 
 instrumentation and coverage mapping data generation.
 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
 
 B. Proposed new behavior:
 
 
 
 1. -fprofile-instr-generate turns on IR late instrumentation
 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
 4. -fprofile-instr-use=<> will automatically determine how to use the 
 profile data.
>>> 
>>> 
>>> Very good observation that we can determine FE or IR automatically based on 
>>> the input profdata. That simplifies things.
>>> 
 B.2) above can be done today for improved usability.
>>> 
>>> 
>>> I don't see how this improves usability. In fact, it is confusing because 
>>> it hijacks an existing option.
>> 
>> Hijacking an existing option to do something different would definitely be a 
>> problem. Please find a way to specify IR-level instrumentation without 
>> breaking compatibility. If you want to replace the existing options with 
>> something different, we’ll need a transition period of at least 1-2 LLVM 
>> releases to migrate.
>> 
> 
> If we remove B.3 above,  then the proposed change (B.2) is essentially
> making '-fcoverage-mapping' an alias to '-fprofile-instr-generate
> -fcoverage-mapping'.   No existing workflow will be broken and new
> users can take advantage of the shortened option.  The reason is that
> there will be no users that only use -fcoverage-mapping option alone
> and rely on its behavior (which is clang error).

The part I’m concerned about is B.1. The current behavior is that 
-fprofile-instr-generate enabled FE instrumentation. We can’t hijack that to do 
something different, at least without a sufficiently long transition period for 
clients to adapt. We use that to generate PGO profiles even when not using 
-fcoverage-mapping.

> 
> 
>>> 
>>> Also B.3 causes existing user builds to emit a warning, which is annoying.
>>> 
>>> I would propose the following modification of B:
>>> 
>>> C.:
>>> 
>>> 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves 
>>> exactly as before, except that it uses IR instrumentation)
>>> 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend 
>>> instrumentation and coverage. (i.e. behaves exactly as before)
>>> 3. -fprofile-instr-use=<> automatically determines which method to use
>>> 
>>> All existing user workflows continue to work, except for workflows that 
>>> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they 
>>> have checked-in to version control and represents some special workload) 
>>> with the profile data from new binaries.
>> 
>> The coverage mapping adds considerable cost. IR-level instrumentation has 
>> some problems that make it undesirable for our workflow, so we need a way to 
>> select front-end instrumentation without adding a bunch of unnecessary 
>> overhead (generating the coverage mapping when you’re not actually doing 
>> coverage testing). I disagree with your assessment that existing workflows 
>> would continue to “work” because ours would not.
>> 
>>> 
>>> Concretely, imagine the following workflow:
>>> 
>>> clang -fprofile-instr-generate foo.c -o foo
>>> ./foo
>>> llvm-profdata merge default.profraw -o new.profdata
>>> llvm-profdata merge new.profdata 
>>> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
>>> -o foo.profdata
>>> clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
>>> 
>>> I think this is a reasonable breakage. We would need to add a note in the 
>>> release notes. Unfortunately this is not expected breakage if we claim to 
>>> have forward compatibility for profdata (which IIRC Apple requires; 
>>> @bogner?).
>> 
>> Yes, that is a requirement for us. We need existing profdata to work with 
>> newer versions of clang (which is why IR-level instrumentation doesn’t work 
>> for us).
>> 
> 
> profile-use can automatically detect FE based profile data and use it
> 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Bob Wilson via cfe-commits]

> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits 
>  wrote:
> 
> silvas added a comment.
> 
> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
> 
>> For the longer term, one possible solution is to make FE based
>> instrumentation only used for coverage testing which can be turned on
>> with -fcoverage-mapping option (currently, -fcoverage-mapping can not
>> be used alone and must be used together with
>> -fprofile-instr-generate). To summarize:
>> 
>> A. Current behavior:
>> 
>> ---
>> 
>> 1. -fprofile-instr-generate turns on FE based instrumentation
>> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based 
>> instrumentation and coverage mapping data generation.
>> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>> 
>> B. Proposed new behavior:
>> 
>> 
>> 
>> 1. -fprofile-instr-generate turns on IR late instrumentation
>> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
>> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
>> 4. -fprofile-instr-use=<> will automatically determine how to use the 
>> profile data.
> 
> 
> Very good observation that we can determine FE or IR automatically based on 
> the input profdata. That simplifies things.
> 
>> B.2) above can be done today for improved usability.
> 
> 
> I don't see how this improves usability. In fact, it is confusing because it 
> hijacks an existing option.

Hijacking an existing option to do something different would definitely be a 
problem. Please find a way to specify IR-level instrumentation without breaking 
compatibility. If you want to replace the existing options with something 
different, we’ll need a transition period of at least 1-2 LLVM releases to 
migrate.

> 
> Also B.3 causes existing user builds to emit a warning, which is annoying.
> 
> I would propose the following modification of B:
> 
> C.:
> 
> 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves 
> exactly as before, except that it uses IR instrumentation)
> 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend 
> instrumentation and coverage. (i.e. behaves exactly as before)
> 3. -fprofile-instr-use=<> automatically determines which method to use
> 
> All existing user workflows continue to work, except for workflows that 
> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they 
> have checked-in to version control and represents some special workload) with 
> the profile data from new binaries.

The coverage mapping adds considerable cost. IR-level instrumentation has some 
problems that make it undesirable for our workflow, so we need a way to select 
front-end instrumentation without adding a bunch of unnecessary overhead 
(generating the coverage mapping when you’re not actually doing coverage 
testing). I disagree with your assessment that existing workflows would 
continue to “work” because ours would not.

> 
> Concretely, imagine the following workflow:
> 
>  clang -fprofile-instr-generate foo.c -o foo
>  ./foo
>  llvm-profdata merge default.profraw -o new.profdata
>  llvm-profdata merge new.profdata 
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
> -o foo.profdata
>  clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
> 
> I think this is a reasonable breakage. We would need to add a note in the 
> release notes. Unfortunately this is not expected breakage if we claim to 
> have forward compatibility for profdata (which IIRC Apple requires; @bogner?).

Yes, that is a requirement for us. We need existing profdata to work with newer 
versions of clang (which is why IR-level instrumentation doesn’t work for us).

> But I think this case will be rare and exceptional enough that we can 
> tolerate it:
> 
> - a simple immediate workaround is to specify `-fcoverage-mapping` (which 
> also adds some extra stuff, but works around the issue)
> - Presumably 
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
> is regenerated with some frequency which is more frequent than upgrading the 
> compiler, and so it is likely reasonable to regenerate it alongside a 
> compiler upgrade, using the workaround until then.

No, that assumption is not necessarily true for us. We need to be able to 
upgrade the compiler without breaking projects that we don’t control, and that 
includes regressing their performance because of an outdated profile.

> 
> 
> 
>> B.1) needs a
> 
>> transition period before  the IR based instrumentation becomes
> 
>> stablized (and can be flipped to the default).  During the transition
> 
>> period, the behavior of 1) does not change, but a cc1 option can be
> 
>> used to turn on IR instrumentation (as proposed by Sean).
> 
> 
> Just to clarify, users are not allowed to use cc1 options. The cc1 option is 
> purely for us as compiler developers to do integration and 

r257556 - Generalize r256026 to apply to all MachO targets, not just Darwin targets.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Tue Jan 12 19:19:02 2016
New Revision: 257556

URL: http://llvm.org/viewvc/llvm-project?rev=257556=rev
Log:
Generalize r256026 to apply to all MachO targets, not just Darwin targets.

The PIC default is set for the MachO toolchain, not just the Darwin toolchain,
so this treats those the same. The behavior with -static should be the same
for all MachO targets. rdar://24152327

Modified:
cfe/trunk/lib/Driver/Tools.cpp
cfe/trunk/test/Driver/pic.c

Modified: cfe/trunk/lib/Driver/Tools.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=257556=257555=257556=diff
==
--- cfe/trunk/lib/Driver/Tools.cpp (original)
+++ cfe/trunk/lib/Driver/Tools.cpp Tue Jan 12 19:19:02 2016
@@ -3263,8 +3263,9 @@ ParsePICArgs(const ToolChain ,
   // ToolChain.getTriple() and Triple?
   bool PIE = ToolChain.isPIEDefault();
   bool PIC = PIE || ToolChain.isPICDefault();
-  // The Darwin default to use PIC does not apply when using -static.
-  if (ToolChain.getTriple().isOSDarwin() && Args.hasArg(options::OPT_static))
+  // The Darwin/MachO default to use PIC does not apply when using -static.
+  if (ToolChain.getTriple().isOSBinFormatMachO() &&
+  Args.hasArg(options::OPT_static))
 PIE = PIC = false;
   bool IsPICLevelTwo = PIC;
 

Modified: cfe/trunk/test/Driver/pic.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/pic.c?rev=257556=257555=257556=diff
==
--- cfe/trunk/test/Driver/pic.c (original)
+++ cfe/trunk/test/Driver/pic.c Tue Jan 12 19:19:02 2016
@@ -218,6 +218,8 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIC
 // RUN: %clang -c %s -target armv7-apple-ios -fapple-kext 
-miphoneos-version-min=6.0.0 -static -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIC
+// RUN: %clang -c %s -target armv7-apple-unknown-macho -static -### 2>&1 \
+// RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIC
 //
 // On OpenBSD, PIE is enabled by default, but can be disabled.
 // RUN: %clang -c %s -target amd64-pc-openbsd -### 2>&1 \


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r249995 - [Sema] Allow C conversions in C overload logic

[Bob Wilson via cfe-commits]

George,

This change caused a serious regression for Objective-C method lookup. See 
PR26085 (http://llvm.org/pr26085).

For the test case in that PR, Sema::SelectBestMethod looks at the two candidate 
"test" methods. It will match the second one, but in the process of considering 
the first candidate, an error diagnostic is generated. This happens within the 
call to CheckSingleAssignmentConstraints that was added here in 
IsStandardConversion. The "Diagnose" argument in that call is set to false, but 
the diagnostic is generated anyway. For the test case in the PR, the diagnostic 
comes from CheckObjCARCConversion, but it looks like there are some other 
diagnostics that could also be generated from within 
CheckSingleAssignmentConstraints.

I think I could manage a fix, e.g., by threading the “Diagnose” flag through 
all the relevant code and consistently checking it before emitting diagnostics, 
but I’m not especially familiar with this part of clang. If you or someone else 
who knows more about this area can figure out the best way to fix it, I would 
appreciate it.

—Bob

> On Oct 11, 2015, at 1:13 PM, George Burgess IV via cfe-commits 
>  wrote:
> 
> Author: gbiv
> Date: Sun Oct 11 15:13:20 2015
> New Revision: 249995
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=249995=rev
> Log:
> [Sema] Allow C conversions in C overload logic
> 
> C allows for some implicit conversions that C++ does not, e.g. void* ->
> char*. This patch teaches clang that these conversions are okay when
> dealing with overloads in C.
> 
> Differential Revision: http://reviews.llvm.org/D13604
> 
> Modified:
>cfe/trunk/include/clang/Sema/Overload.h
>cfe/trunk/include/clang/Sema/Sema.h
>cfe/trunk/lib/Sema/SemaExpr.cpp
>cfe/trunk/lib/Sema/SemaOverload.cpp
>cfe/trunk/test/Sema/overloadable.c
> 
> Modified: cfe/trunk/include/clang/Sema/Overload.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Overload.h?rev=249995=249994=249995=diff
> ==
> --- cfe/trunk/include/clang/Sema/Overload.h (original)
> +++ cfe/trunk/include/clang/Sema/Overload.h Sun Oct 11 15:13:20 2015
> @@ -83,7 +83,8 @@ namespace clang {
> ICK_TransparentUnionConversion, ///< Transparent Union Conversions
> ICK_Writeback_Conversion,  ///< Objective-C ARC writeback conversion
> ICK_Zero_Event_Conversion, ///< Zero constant to event (OpenCL1.2 6.12.10)
> -ICK_Num_Conversion_Kinds   ///< The number of conversion kinds
> +ICK_C_Only_Conversion, ///< Conversions allowed in C, but not C++
> +ICK_Num_Conversion_Kinds,  ///< The number of conversion kinds
>   };
> 
>   /// ImplicitConversionRank - The rank of an implicit conversion
> @@ -95,7 +96,9 @@ namespace clang {
> ICR_Promotion,   ///< Promotion
> ICR_Conversion,  ///< Conversion
> ICR_Complex_Real_Conversion, ///< Complex <-> Real conversion
> -ICR_Writeback_Conversion ///< ObjC ARC writeback conversion
> +ICR_Writeback_Conversion,///< ObjC ARC writeback conversion
> +ICR_C_Conversion ///< Conversion only allowed in the C 
> standard.
> + ///  (e.g. void* to char*)
>   };
> 
>   ImplicitConversionRank GetConversionRank(ImplicitConversionKind Kind);
> 
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=249995=249994=249995=diff
> ==
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Sun Oct 11 15:13:20 2015
> @@ -8292,19 +8292,23 @@ public:
>QualType LHSType,
>QualType RHSType);
> 
> -  /// Check assignment constraints and prepare for a conversion of the
> -  /// RHS to the LHS type.
> +  /// Check assignment constraints and optionally prepare for a conversion of
> +  /// the RHS to the LHS type. The conversion is prepared for if ConvertRHS
> +  /// is true.
>   AssignConvertType CheckAssignmentConstraints(QualType LHSType,
>ExprResult ,
> -   CastKind );
> +   CastKind ,
> +   bool ConvertRHS = true);
> 
>   // CheckSingleAssignmentConstraints - Currently used by
>   // CheckAssignmentOperands, and ActOnReturnStmt. Prior to type checking,
> -  // this routine performs the default function/array converions.
> +  // this routine performs the default function/array converions, if 
> ConvertRHS
> +  // is true.
>   AssignConvertType CheckSingleAssignmentConstraints(QualType LHSType,
>  ExprResult ,
>   

Re: [PATCH] D15195: PR4941: Add support for -fno-builtin-foo options.

[Bob Wilson via cfe-commits]

bob.wilson accepted this revision.
bob.wilson added a comment.
This revision is now accepted and ready to land.

This looks good to me. Thanks for working on this!


http://reviews.llvm.org/D15195



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15455: [Driver] Let -static override the toolchain default PIC setting.

[Bob Wilson via cfe-commits]

bob.wilson accepted this revision.
bob.wilson added a reviewer: bob.wilson.
bob.wilson added a comment.
This revision is now accepted and ready to land.

I applied a Darwin-specific change for this to clang in r256026.


http://reviews.llvm.org/D15455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r256026 - PIC should not be enabled by default on Darwin with -static.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Fri Dec 18 14:37:54 2015
New Revision: 256026

URL: http://llvm.org/viewvc/llvm-project?rev=256026=rev
Log:
PIC should not be enabled by default on Darwin with -static.

r245667 changed -static so that it doesn't override an explicit -fPIC
option, but -static should still change the default for Darwin for -fno-PIC.
This matches longstanding GCC and Clang behavior on Darwin and changing it
would be disruptive, with no significant benefit.
http://reviews.llvm.org/D15455
rdar://problem/23811045

Modified:
cfe/trunk/lib/Driver/Tools.cpp
cfe/trunk/test/Driver/pic.c

Modified: cfe/trunk/lib/Driver/Tools.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=256026=256025=256026=diff
==
--- cfe/trunk/lib/Driver/Tools.cpp (original)
+++ cfe/trunk/lib/Driver/Tools.cpp Fri Dec 18 14:37:54 2015
@@ -3226,6 +3226,9 @@ ParsePICArgs(const ToolChain ,
   // ToolChain.getTriple() and Triple?
   bool PIE = ToolChain.isPIEDefault();
   bool PIC = PIE || ToolChain.isPICDefault();
+  // The Darwin default to use PIC does not apply when using -static.
+  if (ToolChain.getTriple().isOSDarwin() && Args.hasArg(options::OPT_static))
+PIE = PIC = false;
   bool IsPICLevelTwo = PIC;
 
   bool KernelOrKext =

Modified: cfe/trunk/test/Driver/pic.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/pic.c?rev=256026=256025=256026=diff
==
--- cfe/trunk/test/Driver/pic.c (original)
+++ cfe/trunk/test/Driver/pic.c Fri Dec 18 14:37:54 2015
@@ -217,7 +217,7 @@
 // RUN: %clang -c %s -target armv7-apple-ios -fapple-kext 
-miphoneos-version-min=5.0.0 -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIC
 // RUN: %clang -c %s -target armv7-apple-ios -fapple-kext 
-miphoneos-version-min=6.0.0 -static -### 2>&1 \
-// RUN:   | FileCheck %s --check-prefix=CHECK-PIC2
+// RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIC
 //
 // On OpenBSD, PIE is enabled by default, but can be disabled.
 // RUN: %clang -c %s -target amd64-pc-openbsd -### 2>&1 \


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15455: [Driver] Let -static override the toolchain default PIC setting.

[Bob Wilson via cfe-commits]

> On Dec 17, 2015, at 10:59 AM, Bob Wilson via cfe-commits 
> <cfe-commits@lists.llvm.org> wrote:
> 
> 
>> On Dec 17, 2015, at 10:16 AM, Joerg Sonnenberger via cfe-commits 
>> <cfe-commits@lists.llvm.org> wrote:
>> 
>> On Wed, Dec 16, 2015 at 11:59:10PM +, Bob Wilson via cfe-commits wrote:
>>> We can change this to be Darwin-specific if you prefer, but we should
>>> maintain compatibility with GCC and previous Clang releases in this 
>>> behavior.
>> 
>> Who is really affected by this? I don't care too much about obscure
>> Darwin hacks, but I really wonder why it isn't better to just explicitly
>> add -fno-PIC (e.g. when building a kernel module). It's not like that
>> will break on older versions of GCC or Clang.
> 
> Apple has internal projects that are failing to build. This behavior has been 
> in places for many years and I don’t even know how we could find all the 
> people relying on this behavior. Yes, we could break them and force everyone 
> to add -fno-PIC, but typically when we make disruptive and incompatible 
> changes like that, we need to stage the changes and give people a transition 
> plan. For example, we could keep the old behavior but add a warning about the 
> change, something like “warning: -static may be changed in future versions of 
> clang to stop implying -fno-PIC”. After a year or two, we could then go ahead 
> with the change. That is all a lot of work and there needs to be some 
> significant benefit to justify breaking compatibility with older compilers. I 
> don’t see any significant benefit here. It’s a 2-line change to the driver.

Joerg, I’m going to interpret your “I don’t care too much” comment as an 
indication that you’re not opposed to moving forward with the Darwin-specific 
change to restore the previous behavior. I went ahead and committed the change 
in r256026.

I can also add another data point on the impact. We just spent several days 
tracking down a problem that turned out to be caused by this. The code built 
successfully but crashed at run-time. It was extremely difficult to figure out 
what was going wrong.

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15455: [Driver] Let -static override the toolchain default PIC setting.

[Bob Wilson via cfe-commits]

> On Dec 17, 2015, at 10:16 AM, Joerg Sonnenberger via cfe-commits 
> <cfe-commits@lists.llvm.org> wrote:
> 
> On Wed, Dec 16, 2015 at 11:59:10PM +0000, Bob Wilson via cfe-commits wrote:
>> We can change this to be Darwin-specific if you prefer, but we should
>> maintain compatibility with GCC and previous Clang releases in this behavior.
> 
> Who is really affected by this? I don't care too much about obscure
> Darwin hacks, but I really wonder why it isn't better to just explicitly
> add -fno-PIC (e.g. when building a kernel module). It's not like that
> will break on older versions of GCC or Clang.

Apple has internal projects that are failing to build. This behavior has been 
in places for many years and I don’t even know how we could find all the people 
relying on this behavior. Yes, we could break them and force everyone to add 
-fno-PIC, but typically when we make disruptive and incompatible changes like 
that, we need to stage the changes and give people a transition plan. For 
example, we could keep the old behavior but add a warning about the change, 
something like “warning: -static may be changed in future versions of clang to 
stop implying -fno-PIC”. After a year or two, we could then go ahead with the 
change. That is all a lot of work and there needs to be some significant 
benefit to justify breaking compatibility with older compilers. I don’t see any 
significant benefit here. It’s a 2-line change to the driver.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15455: [Driver] Let -static override the toolchain default PIC setting.

[Bob Wilson via cfe-commits]

bob.wilson added a subscriber: bob.wilson.
bob.wilson added a comment.

There has been a long-standing convention in both gcc and clang for Darwin that 
-static changes the default for PIC. Changing that convention is breaking stuff 
for us. The commit message for r245667 says "This new behavior also matches 
GCC", but that is not correct, at least for Darwin. I just tried compiling a 
simple test file with gcc-4.8.2 for OS X. If you run "gcc -v" with and without 
-static, you'll see that the gcc driver adds -fPIC to the cc1 command only when 
you do not use -static. I only have access to the GCC sources from an old 
version, but the config/i386/darwin.h file has this:

/* We want -fPIC by default, unless we're using -static to compile for

  the kernel or some such.  */

#undef CC1_SPEC
#define CC1_SPEC "%{!mkernel:%{!static:%{!mdynamic-no-pic:-fPIC}}}


We can change this to be Darwin-specific if you prefer, but we should maintain 
compatibility with GCC and previous Clang releases in this behavior.


http://reviews.llvm.org/D15455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r252960 - [modules] Simplify and generalize the existing rule for finding hidden

[Bob Wilson via cfe-commits]

> On Nov 12, 2015, at 2:19 PM, Richard Smith via cfe-commits 
>  wrote:
> 
> Author: rsmith
> Date: Thu Nov 12 16:19:45 2015
> New Revision: 252960
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=252960=rev
> Log:
> [modules] Simplify and generalize the existing rule for finding hidden
> declarations in redeclaration lookup. A declaration is now visible to
> lookup if:
> 
> * It is visible (not in a module, or in an imported module), or
> * We're doing redeclaration lookup and it's externally-visible, or
> * We're doing typo correction and looking for unimported decls.
> 
> We now support multiple modules having different internal-linkage or 
> no-linkage
> definitions of the same name for all entities, not just for functions,
> variables, and some typedefs. As previously, if multiple such entities are
> visible, any attempt to use them will result in an ambiguity error.
> 
> This patch fixes the linkage calculation for a number of entities where we
> previously didn't need to get it right (using-declarations, namespace aliases,
> and so on).  It also classifies enumerators as always having no linkage, which
> is a slight deviation from the C++ standard's definition, but not an 
> observable
> change outside modules (this change is being discussed on the -core reflector
> currently).
> 
> This also removes the prior special case for tag lookup, which made some cases
> of this work, but also led to bizarre, bogus "must use 'struct' to refer to 
> type
> 'Foo' in this scope" diagnostics in C++.

We’re seeing a build failure that seems like it is due to this change. The 
following code used to compile successfully:

namespace llvm {
template  class AllocatorBase {};
namespace filter {
class Node {
  class NodeBits {};
  class UniformBits {};
  union {
UniformBits UniformBits;
  };
  static_assert(sizeof(UniformBits) <= 8, "fits in an uint64_6");
};
}
}

but now we get "error: reference to 'UniformBits' is ambiguous” from the 
static_assert. It looks to me like this really is ambiguous and that the code 
should be changed. Can you confirm that?

I also noticed that we get a duplicated diagnostic in this case. I noticed that 
you fixed a related case in r252967, but it seems to be missing this case.

> 
> Added:
>cfe/trunk/test/Modules/Inputs/no-linkage/
>cfe/trunk/test/Modules/Inputs/no-linkage/decls.h
>cfe/trunk/test/Modules/Inputs/no-linkage/empty.h
>cfe/trunk/test/Modules/Inputs/no-linkage/module.modulemap
>cfe/trunk/test/Modules/no-linkage.cpp
> Modified:
>cfe/trunk/include/clang/Sema/Lookup.h
>cfe/trunk/lib/AST/Decl.cpp
>cfe/trunk/lib/Sema/SemaDecl.cpp
>cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>cfe/trunk/test/Index/linkage.c
>cfe/trunk/test/Index/usrs.m
>cfe/trunk/test/Modules/decldef.m
>cfe/trunk/test/Modules/merge-enumerators.cpp
>cfe/trunk/test/Modules/module-private.cpp
>cfe/trunk/test/Modules/submodule-visibility-cycles.cpp
>cfe/trunk/test/Modules/submodules-merge-defs.cpp
> 
> Modified: cfe/trunk/include/clang/Sema/Lookup.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Lookup.h?rev=252960=252959=252960=diff
> ==
> --- cfe/trunk/include/clang/Sema/Lookup.h (original)
> +++ cfe/trunk/include/clang/Sema/Lookup.h Thu Nov 12 16:19:45 2015
> @@ -139,8 +139,7 @@ public:
>   Redecl(Redecl != Sema::NotForRedeclaration),
>   HideTags(true),
>   Diagnose(Redecl == Sema::NotForRedeclaration),
> -  AllowHidden(Redecl == Sema::ForRedeclaration),
> -  AllowHiddenInternal(AllowHidden),
> +  AllowHidden(false),
>   Shadowed(false)
>   {
> configure();
> @@ -162,8 +161,7 @@ public:
>   Redecl(Redecl != Sema::NotForRedeclaration),
>   HideTags(true),
>   Diagnose(Redecl == Sema::NotForRedeclaration),
> -  AllowHidden(Redecl == Sema::ForRedeclaration),
> -  AllowHiddenInternal(AllowHidden),
> +  AllowHidden(false),
>   Shadowed(false)
>   {
> configure();
> @@ -184,7 +182,6 @@ public:
>   HideTags(Other.HideTags),
>   Diagnose(false),
>   AllowHidden(Other.AllowHidden),
> -  AllowHiddenInternal(Other.AllowHiddenInternal),
>   Shadowed(false)
>   {}
> 
> @@ -226,27 +223,16 @@ public:
>   /// \brief Specify whether hidden declarations are visible, e.g.,
>   /// for recovery reasons.
>   void setAllowHidden(bool AH) {
> -AllowHiddenInternal = AllowHidden = AH;
> -  }
> -
> -  /// \brief Specify whether hidden internal declarations are visible.
> -  void setAllowHiddenInternal(bool AHI) {
> -AllowHiddenInternal = AHI;
> +AllowHidden = AH;
>   }
> 
>   /// \brief Determine whether this lookup is permitted to see hidden
>   /// declarations, such as those in modules that have not yet been imported.
>   bool isHiddenDeclarationVisible(NamedDecl *ND) const {
> -// If a using-shadow declaration is hidden, it's never visible, not
> -   

Re: [PATCH] D15195: PR4941: Add support for -fno-builtin-foo options.

[Bob Wilson via cfe-commits]

bob.wilson added a comment.

Regarding the FIXME in lib/Frontend/CompilerInvocation.cpp: I agree with Hal 
that you can remove that. We used to complain about unsupported -fno-builtin-* 
options (and until now they have *all* been unsupported), but in r191434, 
Rafael changed clang to silently ignore those options, with the explanation 
that it matches gcc's behavior. Assuming that gcc has not changed in that 
regard, it makes sense that we should continue to ignore -fno-builtin-* for 
unsupported options.


http://reviews.llvm.org/D15195



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r248975 - Fix printing of parameterized Objective-C interfaces.

[Bob Wilson via cfe-commits]

Author: bwilson
Date: Wed Sep 30 19:53:13 2015
New Revision: 248975

URL: http://llvm.org/viewvc/llvm-project?rev=248975=rev
Log:
Fix printing of parameterized Objective-C interfaces.

This change was accidentally omitted from Doug's change in r241541.

Modified:
cfe/trunk/lib/AST/DeclPrinter.cpp
cfe/trunk/test/Index/comment-objc-parameterized-classes.m

Modified: cfe/trunk/lib/AST/DeclPrinter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclPrinter.cpp?rev=248975=248974=248975=diff
==
--- cfe/trunk/lib/AST/DeclPrinter.cpp (original)
+++ cfe/trunk/lib/AST/DeclPrinter.cpp Wed Sep 30 19:53:13 2015
@@ -1088,7 +1088,7 @@ void DeclPrinter::VisitObjCInterfaceDecl
   }
   
   if (SID)
-Out << " : " << OID->getSuperClass()->getName();
+Out << " : " << QualType(OID->getSuperClassType(), 0).getAsString(Policy);
 
   // Protocols?
   const ObjCList  = OID->getReferencedProtocols();

Modified: cfe/trunk/test/Index/comment-objc-parameterized-classes.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/comment-objc-parameterized-classes.m?rev=248975=248974=248975=diff
==
--- cfe/trunk/test/Index/comment-objc-parameterized-classes.m (original)
+++ cfe/trunk/test/Index/comment-objc-parameterized-classes.m Wed Sep 30 
19:53:13 2015
@@ -17,3 +17,8 @@
 /// A
 @interface A<__covariant T : id, U : NSObject *> : NSObject
 @end
+
+// CHECK: @interface AA : A id, NSObject *
+/// AA
+@interface AA : A
+@end


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13113: [darwin] [builtins] Stop generating cc_kext_ios5 and move iOS architectures out of cc_kext into cc_kext_ios

[Bob Wilson via cfe-commits]

bob.wilson accepted this revision.
bob.wilson added a comment.
This revision is now accepted and ready to land.

Good idea. The patch looks good. It will need to be coordinated with a change 
to compiler_rt build cc_kext_ios.a, though. Thanks for cleaning this up.


http://reviews.llvm.org/D13113



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits