[PATCH] D46767: Force the PS4 clang ABI version to 6, and add a warning if this is attempted to be overridden

[Paul Robinson via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D46767



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

[PATCH] D46767: Force the PS4 clang ABI version to 6, and add a warning if this is attempted to be overridden

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D46767#1096746, @rsmith wrote:

> Everything old is new again.


If only that were true about my brain. :-P

> This was discussed when `-fclang-abi-compat` was introduced; see 
> https://reviews.llvm.org/D36501 for the argument why this patch is the wrong 
> way of modeling an ABI. Forcing the `ClangABICompat` language option as a way 
> of "tricking" Clang into producing the PS4 ABI is a hack. The various ABI 
> changes that `-fclang-abi-compat=` controls are simply part of the PS4 ABI, 
> and that knowledge should idealistically be carried by the CodeGen (etc) code 
> that knows about PS4, rather than by imagining that there is some other PS4 
> ABI that Clang would produces at version `Latest`, and that we're asking for 
> a compatibility version of it.

Muchas gracias for the reminder of the previous discussion; it's quite true 
that on our side we have not done our due diligence in making sure that 
upstream Clang fully supports the PS4 ABI, and that `-fclang-abi-compat` is the 
wrong way to do this.  It needs to become part of my team's consciousness and 
collective memory that these sorts of expedient hacks are the wrong approach.

> This will go wrong if we ever release (or have ever released) a Clang version 
> that fails to properly implement the PS4 ABI.

Yeah, we crossed that bridge years ago, but nobody has been brave enough to try 
to deliver that patch upstream.  Actually I think there are two, but as they 
typically don't cause any merge conflicts they're not at top-of-mind for 
anybody, even me.

> However, we should not issue a warning for use of the flag. Remember that the 
> flag means "please be ABI-compatible with Clang version X.Y". Because all 
> versions of Clang that target PS4 use the same ABI, the flag is a no-op on 
> that target (at least for now, until we accidentally introduce an ABI break). 
> So we should not be warning on it, just silently accepting it and doing what 
> it says -- which for now is nothing.

Got it.  I'll take an action to straighten this one out.


Repository:
  rC Clang

https://reviews.llvm.org/D46767



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

[PATCH] D46982: Do not enable RTTI with -fexceptions, for PS4

[Paul Robinson via Phabricator via cfe-commits]

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

So on PS4, `-fsanitize=vptr` without an explicit `-frtti` will warn and disable 
the sanitizer, rather than implicitly enabling RTTI.  As well as exceptions no 
longer implicitly enabling full RTTI.
In short, we never implicitly enable RTTI, which was the intention.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D46982



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

[PATCH] D46439: [Sema] Fix incorrect packed aligned structure layout

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

This wouldn't be another layout/ABI breakage, would it?


Repository:
  rC Clang

https://reviews.llvm.org/D46439



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

[PATCH] D47161: update

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I take it this was accidental?  If there are weaknesses in our documentation 
for how to use Phabricator, please let us know.  I know it is not always 
straightforward (I had a number of issues when I tried to start using it).


Repository:
  rC Clang

https://reviews.llvm.org/D47161



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

[PATCH] D47260: Testcase for dwarf 5 crash/assert when calculating a checksum for an expansion

[Paul Robinson via Phabricator via cfe-commits]

probinson added a reviewer: rnk.
probinson added a subscriber: rnk.
probinson added a comment.

+ @rnk who did the initial checksum stuff for CodeView.

I am unclear how the notion of checksums should interact with preprocessed 
files.

Nit: while many tests have dates in the filenames, we no longer use that 
convention.


Repository:
  rC Clang

https://reviews.llvm.org/D47260



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

[PATCH] D47260: Testcase for dwarf 5 crash/assert when calculating a checksum for an expansion

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

After thinking about this a bit...  The # directive will cause the filename to 
be identified as the source for the subsequent lines.  That means it will show 
up as the original source file in the line table.
However, the compiler doesn't have the actual source file of the header (or the 
original source file, for that matter) and so cannot compute a correct checksum 
for it.  That means, it should omit the checksum, for any file identified with 
a # directive.
I haven't looked at how CodeView handles this situation; for DWARF v5 I made 
inconsistent use of a checksum into an assertion offense.
Should we reconsider that?  Or have the # directive cause Clang to omit 
checksums for *all* files, even the one it is actually reading?
@aprantl, @rnk, thoughts?


Repository:
  rC Clang

https://reviews.llvm.org/D47260



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

[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Can we step back a second and better explain what the problem is?  With current 
Clang the debug info for this function looks okay to me.
The store that is "missing" a debug location is homing the formal parameter to 
its local stack location; this is part of prolog setup, not "real" code.


Repository:
  rC Clang

https://reviews.llvm.org/D47097



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

[PATCH] D47260: Testcase for dwarf 5 crash/assert when calculating a checksum for an expansion

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I see that the per-file info does track the presence of `# [line] N` but it's 
not immediately obvious how to query "has any file seen one" which is really 
what I'd want to know.  The file information has various levels of 
indirection...


Repository:
  rC Clang

https://reviews.llvm.org/D47260



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

[PATCH] D47375: [Driver] Add flag "--dependent-lib=..." when enabling asan or ubsan on PS4.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

LGTM with the indicated test tweak, but best if @filcab also takes a look.




Comment at: lib/Driver/ToolChains/PS4CPU.cpp:87
+CmdArgs.push_back("--dependent-lib=libSceDbgAddressSanitizer_stub_weak.a");
+  }
+}

Don't bother with braces for a one-line `if` body.



Comment at: test/Driver/fsanitize.c:624
+// CHECK-ASAN-PS4: --dependent-lib=libSceDbgAddressSanitizer_stub_weak.a
 // CHECK-ASAN-PS4-NOT: {{(\.(o|bc)"? |-l).*-lSceDbgAddressSanitizer_stub_weak}}
 // CHECK-ASAN-PS4: -lSceDbgAddressSanitizer_stub_weak

Repeat this NOT line before the new check? to preserve the property described 
in the comment.


Repository:
  rC Clang

https://reviews.llvm.org/D47375



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

[PATCH] D47260: Testcase for dwarf 5 crash/assert when calculating a checksum for an expansion

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

@trixirt do you mind if I commandeer this review?  I think I have a patch.


Repository:
  rC Clang

https://reviews.llvm.org/D47260



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

[PATCH] D47291: Proposal to make rtti errors more generic.

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6729
 def err_no_dynamic_cast_with_fno_rtti : Error<
-  "cannot use dynamic_cast with -fno-rtti">;
+  "use of dynamic_cast requires enabling RTTI">;
 

filcab wrote:
> I'd prefer to have the way to enable RTTI mentioned in the message. Could we 
> have something like `ToolChain::getRTTIMode()` and have a "RTTI was on/of" or 
> "RTTI defaulted to on/off"? That way we'd be able to have a message similar 
> to the current one (mentioning "you passed -fno-rtti") on platforms that 
> default to RTTI=on, and have your updated message (possibly with a mention of 
> "use -frtti") on platforms that default to RTTI=off.
> 
> (This is a minor usability comment about this patch, I don't consider it a 
> blocker or anything)
If the options are spelled differently for clang-cl and we had a way to 
retrieve the appropriate spellings, providing the option to use in the 
diagnostic does seem like a nice touch.


Repository:
  rC Clang

https://reviews.llvm.org/D47291



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

[PATCH] D47260: [DebugInfo] Skip MD5 checksums of preprocessed files

[Paul Robinson via Phabricator via cfe-commits]

probinson updated this revision to Diff 148663.
probinson added a comment.

Upload patch to suppress checksums when we see a preprocessed file.


https://reviews.llvm.org/D47260

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGen/md5-checksum-crash.c


Index: clang/test/CodeGen/md5-checksum-crash.c
===
--- /dev/null
+++ clang/test/CodeGen/md5-checksum-crash.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited 
-dwarf-version=5 %s -emit-llvm -o- | FileCheck %s
+// RUN: %clang_cc1 -triple %ms_abi_triple -gcodeview -debug-info-kind=limited 
%s -emit-llvm -o- | FileCheck %s
+
+// This had been crashing, no MD5 checksum for string.h.
+// Now if there are #line directives, don't bother with checksums
+// as a preprocessed file won't properly reflect the original source.
+#define __NTH fct
+void fn1() {}
+# 7 "/usr/include/string.h"
+void __NTH() {}
+// Verify no checksum attributes on these files.
+// CHECK-DAG: DIFile(filename: "{{.*}}.c", directory: "{{[^"]*}}")
+// CHECK-DAG: DIFile(filename: "{{.*}}string.h", directory: "{{[^"]*}}")
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -57,6 +57,7 @@
   CodeGenModule &CGM;
   const codegenoptions::DebugInfoKind DebugKind;
   bool DebugTypeExtRefs;
+  mutable bool EmitFileChecksums;
   llvm::DIBuilder DBuilder;
   llvm::DICompileUnit *TheCU = nullptr;
   ModuleMap *ClangModuleMap = nullptr;
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -67,6 +67,8 @@
   DBuilder(CGM.getModule()) {
   for (const auto &KV : CGM.getCodeGenOpts().DebugPrefixMap)
 DebugPrefixMap[KV.first] = KV.second;
+  EmitFileChecksums = CGM.getCodeGenOpts().EmitCodeView ||
+  CGM.getCodeGenOpts().DwarfVersion >= 5;
   CreateCompileUnit();
 }
 
@@ -365,15 +367,19 @@
 CGDebugInfo::computeChecksum(FileID FID, SmallString<32> &Checksum) const {
   Checksum.clear();
 
-  if (!CGM.getCodeGenOpts().EmitCodeView &&
-  CGM.getCodeGenOpts().DwarfVersion < 5)
+  if (!EmitFileChecksums)
 return None;
 
   SourceManager &SM = CGM.getContext().getSourceManager();
   bool Invalid;
-  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID, &Invalid);
-  if (Invalid)
+  const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
+  if (Invalid || !Entry.isFile())
 return None;
+  if (Entry.getFile().hasLineDirectives()) {
+EmitFileChecksums = false;
+return None;
+  }
+  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID);
 
   llvm::MD5 Hash;
   llvm::MD5::MD5Result Result;


Index: clang/test/CodeGen/md5-checksum-crash.c
===
--- /dev/null
+++ clang/test/CodeGen/md5-checksum-crash.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited -dwarf-version=5 %s -emit-llvm -o- | FileCheck %s
+// RUN: %clang_cc1 -triple %ms_abi_triple -gcodeview -debug-info-kind=limited %s -emit-llvm -o- | FileCheck %s
+
+// This had been crashing, no MD5 checksum for string.h.
+// Now if there are #line directives, don't bother with checksums
+// as a preprocessed file won't properly reflect the original source.
+#define __NTH fct
+void fn1() {}
+# 7 "/usr/include/string.h"
+void __NTH() {}
+// Verify no checksum attributes on these files.
+// CHECK-DAG: DIFile(filename: "{{.*}}.c", directory: "{{[^"]*}}")
+// CHECK-DAG: DIFile(filename: "{{.*}}string.h", directory: "{{[^"]*}}")
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -57,6 +57,7 @@
   CodeGenModule &CGM;
   const codegenoptions::DebugInfoKind DebugKind;
   bool DebugTypeExtRefs;
+  mutable bool EmitFileChecksums;
   llvm::DIBuilder DBuilder;
   llvm::DICompileUnit *TheCU = nullptr;
   ModuleMap *ClangModuleMap = nullptr;
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -67,6 +67,8 @@
   DBuilder(CGM.getModule()) {
   for (const auto &KV : CGM.getCodeGenOpts().DebugPrefixMap)
 DebugPrefixMap[KV.first] = KV.second;
+  EmitFileChecksums = CGM.getCodeGenOpts().EmitCodeView ||
+  CGM.getCodeGenOpts().DwarfVersion >= 5;
   CreateCompileUnit();
 }
 
@@ -365,15 +367,19 @@
 CGDebugInfo::computeChecksum(FileID FID, SmallString<32> &Checksum) const {
   Checksum.clear();
 
-  if (!CGM.getCodeGenOpts().EmitCodeView &&
-  CGM.getCodeGenOpts().DwarfVersion < 5)
+  if (!EmitFileChecksums)
 return None;
 
   SourceManager &SM = CGM.

[PATCH] D47260: [DebugInfo] Skip MD5 checksums of preprocessed files

[Paul Robinson via Phabricator via cfe-commits]

probinson marked an inline comment as done.
probinson added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:378
 return None;
+  if (Entry.getFile().hasLineDirectives()) {
+EmitFileChecksums = false;

aprantl wrote:
> Can you add a comment explaining why we are doing this here?
Of course.


https://reviews.llvm.org/D47260



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

[PATCH] D47260: [DebugInfo] Skip MD5 checksums of preprocessed files

[Paul Robinson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
probinson marked an inline comment as done.
Closed by commit rC11: [DebugInfo] Don't bother with MD5 checksums of 
preprocessed files. (authored by probinson, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D47260?vs=148663&id=148667#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D47260

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGen/md5-checksum-crash.c


Index: test/CodeGen/md5-checksum-crash.c
===
--- test/CodeGen/md5-checksum-crash.c
+++ test/CodeGen/md5-checksum-crash.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited 
-dwarf-version=5 %s -emit-llvm -o- | FileCheck %s
+// RUN: %clang_cc1 -triple %ms_abi_triple -gcodeview -debug-info-kind=limited 
%s -emit-llvm -o- | FileCheck %s
+
+// This had been crashing, no MD5 checksum for string.h.
+// Now if there are #line directives, don't bother with checksums
+// as a preprocessed file won't properly reflect the original source.
+#define __NTH fct
+void fn1() {}
+# 7 "/usr/include/string.h"
+void __NTH() {}
+// Verify no checksum attributes on these files.
+// CHECK-DAG: DIFile(filename: "{{.*}}.c", directory: "{{[^"]*}}")
+// CHECK-DAG: DIFile(filename: "{{.*}}string.h", directory: "{{[^"]*}}")
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -67,6 +67,8 @@
   DBuilder(CGM.getModule()) {
   for (const auto &KV : CGM.getCodeGenOpts().DebugPrefixMap)
 DebugPrefixMap[KV.first] = KV.second;
+  EmitFileChecksums = CGM.getCodeGenOpts().EmitCodeView ||
+  CGM.getCodeGenOpts().DwarfVersion >= 5;
   CreateCompileUnit();
 }
 
@@ -365,15 +367,21 @@
 CGDebugInfo::computeChecksum(FileID FID, SmallString<32> &Checksum) const {
   Checksum.clear();
 
-  if (!CGM.getCodeGenOpts().EmitCodeView &&
-  CGM.getCodeGenOpts().DwarfVersion < 5)
+  if (!EmitFileChecksums)
 return None;
 
   SourceManager &SM = CGM.getContext().getSourceManager();
   bool Invalid;
-  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID, &Invalid);
-  if (Invalid)
+  const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
+  if (Invalid || !Entry.isFile())
 return None;
+  if (Entry.getFile().hasLineDirectives()) {
+// This must be a preprocessed file; its content won't match the original
+// source; therefore checksumming the text we have is pointless or wrong.
+EmitFileChecksums = false;
+return None;
+  }
+  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID);
 
   llvm::MD5 Hash;
   llvm::MD5::MD5Result Result;
Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -57,6 +57,7 @@
   CodeGenModule &CGM;
   const codegenoptions::DebugInfoKind DebugKind;
   bool DebugTypeExtRefs;
+  mutable bool EmitFileChecksums;
   llvm::DIBuilder DBuilder;
   llvm::DICompileUnit *TheCU = nullptr;
   ModuleMap *ClangModuleMap = nullptr;


Index: test/CodeGen/md5-checksum-crash.c
===
--- test/CodeGen/md5-checksum-crash.c
+++ test/CodeGen/md5-checksum-crash.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited -dwarf-version=5 %s -emit-llvm -o- | FileCheck %s
+// RUN: %clang_cc1 -triple %ms_abi_triple -gcodeview -debug-info-kind=limited %s -emit-llvm -o- | FileCheck %s
+
+// This had been crashing, no MD5 checksum for string.h.
+// Now if there are #line directives, don't bother with checksums
+// as a preprocessed file won't properly reflect the original source.
+#define __NTH fct
+void fn1() {}
+# 7 "/usr/include/string.h"
+void __NTH() {}
+// Verify no checksum attributes on these files.
+// CHECK-DAG: DIFile(filename: "{{.*}}.c", directory: "{{[^"]*}}")
+// CHECK-DAG: DIFile(filename: "{{.*}}string.h", directory: "{{[^"]*}}")
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -67,6 +67,8 @@
   DBuilder(CGM.getModule()) {
   for (const auto &KV : CGM.getCodeGenOpts().DebugPrefixMap)
 DebugPrefixMap[KV.first] = KV.second;
+  EmitFileChecksums = CGM.getCodeGenOpts().EmitCodeView ||
+  CGM.getCodeGenOpts().DwarfVersion >= 5;
   CreateCompileUnit();
 }
 
@@ -365,15 +367,21 @@
 CGDebugInfo::computeChecksum(FileID FID, SmallString<32> &Checksum) const {
   Checksum.clear();
 
-  if (!CGM.getCodeGenOpts().EmitCodeView &&
-  CGM.getCodeGenOpts().DwarfVersion < 5)
+  if (!EmitFileChecksums)
 return None;
 
   SourceManager &SM = CGM.getContext().getSourceManager();
   bool Invalid;
-  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID, &Invalid);
- 

[PATCH] D47260: [DebugInfo] Skip MD5 checksums of preprocessed files

[Paul Robinson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
probinson marked an inline comment as done.
Closed by commit rL11: [DebugInfo] Don't bother with MD5 checksums of 
preprocessed files. (authored by probinson, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D47260?vs=148663&id=148668#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D47260

Files:
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/lib/CodeGen/CGDebugInfo.h
  cfe/trunk/test/CodeGen/md5-checksum-crash.c


Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -67,6 +67,8 @@
   DBuilder(CGM.getModule()) {
   for (const auto &KV : CGM.getCodeGenOpts().DebugPrefixMap)
 DebugPrefixMap[KV.first] = KV.second;
+  EmitFileChecksums = CGM.getCodeGenOpts().EmitCodeView ||
+  CGM.getCodeGenOpts().DwarfVersion >= 5;
   CreateCompileUnit();
 }
 
@@ -365,15 +367,21 @@
 CGDebugInfo::computeChecksum(FileID FID, SmallString<32> &Checksum) const {
   Checksum.clear();
 
-  if (!CGM.getCodeGenOpts().EmitCodeView &&
-  CGM.getCodeGenOpts().DwarfVersion < 5)
+  if (!EmitFileChecksums)
 return None;
 
   SourceManager &SM = CGM.getContext().getSourceManager();
   bool Invalid;
-  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID, &Invalid);
-  if (Invalid)
+  const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
+  if (Invalid || !Entry.isFile())
 return None;
+  if (Entry.getFile().hasLineDirectives()) {
+// This must be a preprocessed file; its content won't match the original
+// source; therefore checksumming the text we have is pointless or wrong.
+EmitFileChecksums = false;
+return None;
+  }
+  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID);
 
   llvm::MD5 Hash;
   llvm::MD5::MD5Result Result;
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.h
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.h
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.h
@@ -57,6 +57,7 @@
   CodeGenModule &CGM;
   const codegenoptions::DebugInfoKind DebugKind;
   bool DebugTypeExtRefs;
+  mutable bool EmitFileChecksums;
   llvm::DIBuilder DBuilder;
   llvm::DICompileUnit *TheCU = nullptr;
   ModuleMap *ClangModuleMap = nullptr;
Index: cfe/trunk/test/CodeGen/md5-checksum-crash.c
===
--- cfe/trunk/test/CodeGen/md5-checksum-crash.c
+++ cfe/trunk/test/CodeGen/md5-checksum-crash.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited 
-dwarf-version=5 %s -emit-llvm -o- | FileCheck %s
+// RUN: %clang_cc1 -triple %ms_abi_triple -gcodeview -debug-info-kind=limited 
%s -emit-llvm -o- | FileCheck %s
+
+// This had been crashing, no MD5 checksum for string.h.
+// Now if there are #line directives, don't bother with checksums
+// as a preprocessed file won't properly reflect the original source.
+#define __NTH fct
+void fn1() {}
+# 7 "/usr/include/string.h"
+void __NTH() {}
+// Verify no checksum attributes on these files.
+// CHECK-DAG: DIFile(filename: "{{.*}}.c", directory: "{{[^"]*}}")
+// CHECK-DAG: DIFile(filename: "{{.*}}string.h", directory: "{{[^"]*}}")


Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -67,6 +67,8 @@
   DBuilder(CGM.getModule()) {
   for (const auto &KV : CGM.getCodeGenOpts().DebugPrefixMap)
 DebugPrefixMap[KV.first] = KV.second;
+  EmitFileChecksums = CGM.getCodeGenOpts().EmitCodeView ||
+  CGM.getCodeGenOpts().DwarfVersion >= 5;
   CreateCompileUnit();
 }
 
@@ -365,15 +367,21 @@
 CGDebugInfo::computeChecksum(FileID FID, SmallString<32> &Checksum) const {
   Checksum.clear();
 
-  if (!CGM.getCodeGenOpts().EmitCodeView &&
-  CGM.getCodeGenOpts().DwarfVersion < 5)
+  if (!EmitFileChecksums)
 return None;
 
   SourceManager &SM = CGM.getContext().getSourceManager();
   bool Invalid;
-  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID, &Invalid);
-  if (Invalid)
+  const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
+  if (Invalid || !Entry.isFile())
 return None;
+  if (Entry.getFile().hasLineDirectives()) {
+// This must be a preprocessed file; its content won't match the original
+// source; therefore checksumming the text we have is pointless or wrong.
+EmitFileChecksums = false;
+return None;
+  }
+  llvm::MemoryBuffer *MemBuffer = SM.getBuffer(FID);
 
   llvm::MD5 Hash;
   llvm::MD5::MD5Result Result;
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.h
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.h
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.h
@@ -57,6 +57,7 @@
   Cod

[PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Paul Robinson via Phabricator via cfe-commits]

probinson updated this revision to Diff 116711.
probinson added a comment.

Add a command-line flag to control emitting the template parameter children. 
Default to on for SCE debugger tuning.
I am not super excited about my choice of option name or the help text; 
alternate suggestions welcome.

I would prefer to eliminate the `` from the instance name as well, 
because our debugger reconstructs a name more to its liking from the parameter 
children.  However, IIUC the name with params is used for deduplication in LTO, 
so that is probably not such a good idea. :-)


https://reviews.llvm.org/D14358

Files:
  include/clang/Basic/LangOptions.def
  include/clang/Driver/Options.td
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGenCXX/debug-info-fwd-template-param.cpp
  test/Driver/clang_f_opts.c

Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -495,6 +495,11 @@
 // CHECK-PROFILE-DEBUG: -fdebug-info-for-profiling
 // CHECK-NO-PROFILE-DEBUG-NOT: -fdebug-info-for-profiling
 
+// RUN: %clang -### -S -fdebug-forward-template-params %s 2>&1 | FileCheck -check-prefix=CHECK-FWD-TMPL %s
+// RUN: %clang -### -S -fno-debug-forward-template-params %s 2>&1 | FileCheck -check-prefix=CHECK-NO-FWD-TMPL %s
+// CHECK-FWD-TMPL: -fdebug-forward-template-params
+// CHECK-NO-FWD-TMPL-NOT: -fdebug-forward-template-params
+
 // RUN: %clang -### -S -fallow-editor-placeholders %s 2>&1 | FileCheck -check-prefix=CHECK-ALLOW-PLACEHOLDERS %s
 // RUN: %clang -### -S -fno-allow-editor-placeholders %s 2>&1 | FileCheck -check-prefix=CHECK-NO-ALLOW-PLACEHOLDERS %s
 // CHECK-ALLOW-PLACEHOLDERS: -fallow-editor-placeholders
Index: test/CodeGenCXX/debug-info-fwd-template-param.cpp
===
--- test/CodeGenCXX/debug-info-fwd-template-param.cpp
+++ test/CodeGenCXX/debug-info-fwd-template-param.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 %s -triple=%itanium_abi_triple -debug-info-kind=limited -fdebug-forward-template-params -emit-llvm -o - | FileCheck --check-prefix=CHILD %s
+// RUN: %clang_cc1 %s -triple=%itanium_abi_triple -debug-info-kind=limited -fno-debug-forward-template-params -emit-llvm -o - | FileCheck --check-prefix=NONE %s
+// A forward declaration of a template instantiation should have template
+// parameter children (if we ask for them).
+
+template class A {
+public:
+  A(T val);
+private:
+  T x;
+};
+
+struct B {
+  A *p;
+};
+
+B b;
+
+// CHILD:  !DICompositeType(tag: DW_TAG_class_type, name: "A"
+// CHILD-SAME: flags: DIFlagFwdDecl
+// CHILD-SAME: templateParams: [[PARAM_LIST:![0-9]*]]
+// CHILD:  [[PARAM_LIST]] = !{[[PARAM:![0-9]*]]}
+// CHILD:  [[PARAM]] = !DITemplateTypeParameter(name: "T",
+// CHILD-SAME: type: [[CTYPE:![0-9]*]]
+// CHILD:  [[CTYPE]] = !DIDerivedType(tag: DW_TAG_const_type
+// CHILD-SAME: baseType: [[BTYPE:![0-9]*]]
+// CHILD:  [[BTYPE]] = !DIBasicType(name: "int"
+
+// NONE:   !DICompositeType(tag: DW_TAG_class_type, name: "A"
+// NONE-SAME:  flags: DIFlagFwdDecl
+// NONE-NOT:   templateParams:
+// NONE-SAME:  )
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2201,6 +2201,7 @@
   Opts.EncodeExtendedBlockSig =
 Args.hasArg(OPT_fencode_extended_block_signature);
   Opts.EmitAllDecls = Args.hasArg(OPT_femit_all_decls);
+  Opts.EmitFwdTemplateChildren = Args.hasArg(OPT_fdebug_forward_template_params);
   Opts.PackStruct = getLastArgIntValue(Args, OPT_fpack_struct_EQ, 0, Diags);
   Opts.MaxTypeAlign = getLastArgIntValue(Args, OPT_fmax_type_align_EQ, 0, Diags);
   Opts.AlignDouble = Args.hasArg(OPT_malign_double);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2969,6 +2969,13 @@
 CmdArgs.push_back("-generate-type-units");
   }
 
+  // Decide how to render forward declarations of template instantiations.
+  // SCE defaults to on, others default to off.
+  if (Args.hasFlag(options::OPT_fdebug_forward_template_params,
+   options::OPT_fno_debug_forward_template_params,
+   DebuggerTuning == llvm::DebuggerKind::SCE))
+CmdArgs.push_back("-fdebug-forward-template-params");
+
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D);
 }
 
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -833,6 +833,10 @@
   llvm::DICompositeType *RetTy = DBuilder.createReplaceableCompositeType(
   getTagForRecord(RD), RDName, Ctx, DefUnit, Line, 0, Size, Align,
   llvm::DINode::FlagFwdDecl, FullName);
+  if (CG

[PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D14358#881666, @aprantl wrote:

> Does this have to be exposed through the driver or could this be a cc1 option 
> only?


My thinking was to make it easier for LLDB to play with it and decide whether 
DWARF conformance on this point is a good thing for them also.  But I admit 
it's not something an end user would really care about or want to change.  I 
can make it a cc1 option.




Comment at: include/clang/Basic/LangOptions.def:144
 BENIGN_LANGOPT(EmitAllDecls  , 1, 0, "emitting all declarations")
+BENIGN_LANGOPT(EmitFwdTemplateChildren, 1, 0, "emit template parameter 
children in forward declarations")
 LANGOPT(MathErrno , 1, 1, "errno in math functions")

aprantl wrote:
> Why is this a LangOpt instead of a CodeGenOpt?
> Should it reference `debug` somewhere in the name?
Because I thought EmitAllDecls was for debug info, and this felt related.  But 
I see EmitAllDecls is not used in CGDebugInfo and generally we do put 
debug-related options in CodeGenOpt so I will move that.  (Someday we should 
collect all that stuff into a DebugOpt class.)
And I will rename it to DebugFwdTemplateChildren.


https://reviews.llvm.org/D14358



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

[PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D14358#882445, @dblaikie wrote:

> > I would prefer to eliminate the `` from the instance name as well, 
> > because our debugger reconstructs a name more to its liking from the 
> > parameter children.  However, IIUC the name with params is used for 
> > deduplication in LTO, so that is probably not such a good idea. :-)
>
> Though you have this out of tree? How do you cope with LTO there?


We discovered that we had to keep them in the name for LTO.

> I've not fully refreshed myself on the previous conversations - but it looks 
> like my thought was that this state proposed here is weird/inconsistent: The 
> parameters are already in the name, so adding them in the DIEs seems 
> redundant. If the parameters weren't in the name then this change might make 
> more sense.

Our debugger throws away the params in the name, and relies on the children.  
The names as rendered in DWARF by Clang are not textually consistent with names 
as rendered by the demangler.  Our debugger uses the children to construct 
names that are consistent with how the demangler works.  Then it can match up 
type names returned by the demangler to type names it has constructed.  
Assuming I am not misunderstanding our debugger guys again, but that's my 
recollection.

I believe we have talked previously about using a different scheme for 
deduplication that doesn't depend (or not so much) on the names.  If we had 
that, we could eliminate params from the name, and save probably a noticeable 
chunk of space in the string section.  I haven't tried to measure that, though. 
 But we have to have the children in place before we can experiment with other 
deduplication schemes.

There is also the pedantic point that DWARF doesn't say these child entries are 
optional, or omitted for forward declarations.  I know gcc doesn't (or didn't, 
anyway; what I have locally is gcc 5.4) but gcc is not the arbiter of what 
constitutes conforming DWARF.


https://reviews.llvm.org/D14358



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

[PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Paul Robinson via Phabricator via cfe-commits]

probinson updated this revision to Diff 116862.
probinson added a comment.

command-line option is cc1 not driver
internal flag moved from LangOpts to CodeGenOpts and renamed
simplified test


https://reviews.llvm.org/D14358

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGenCXX/debug-info-fwd-template-param.cpp


Index: test/CodeGenCXX/debug-info-fwd-template-param.cpp
===
--- test/CodeGenCXX/debug-info-fwd-template-param.cpp
+++ test/CodeGenCXX/debug-info-fwd-template-param.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 %s -triple=%itanium_abi_triple -debug-info-kind=limited 
-debug-forward-template-params -emit-llvm -o - | FileCheck --check-prefix=CHILD 
%s
+// RUN: %clang_cc1 %s -triple=%itanium_abi_triple -debug-info-kind=limited 
-emit-llvm -o - | FileCheck --check-prefix=NONE %s
+// A DWARF forward declaration of a template instantiation should have template
+// parameter children (if we ask for them).
+
+template class A;
+A *p;
+
+// CHILD:  !DICompositeType(tag: DW_TAG_class_type, name: "A"
+// CHILD-SAME: flags: DIFlagFwdDecl
+// CHILD-SAME: templateParams: [[PARAM_LIST:![0-9]*]]
+// CHILD:  [[PARAM_LIST]] = !{[[PARAM:![0-9]*]]}
+// CHILD:  [[PARAM]] = !DITemplateTypeParameter(name: "T",
+// CHILD-SAME: type: [[CTYPE:![0-9]*]]
+// CHILD:  [[CTYPE]] = !DIDerivedType(tag: DW_TAG_const_type
+// CHILD-SAME: baseType: [[BTYPE:![0-9]*]]
+// CHILD:  [[BTYPE]] = !DIBasicType(name: "int"
+
+// NONE:   !DICompositeType(tag: DW_TAG_class_type, name: "A"
+// NONE-SAME:  flags: DIFlagFwdDecl
+// NONE-NOT:   templateParams:
+// NONE-SAME:  )
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -528,6 +528,7 @@
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
   Opts.DebugTypeExtRefs = Args.hasArg(OPT_dwarf_ext_refs);
   Opts.DebugExplicitImport = Triple.isPS4CPU();
+  Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
 
   for (const auto &Arg : Args.getAllArgValues(OPT_fdebug_prefix_map_EQ))
 Opts.DebugPrefixMap.insert(StringRef(Arg).split('='));
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2969,6 +2969,11 @@
 CmdArgs.push_back("-generate-type-units");
   }
 
+  // Decide how to render forward declarations of template instantiations.
+  // SCE wants full descriptions, others just get them in the name.
+  if (DebuggerTuning == llvm::DebuggerKind::SCE)
+CmdArgs.push_back("-debug-forward-template-params");
+
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D);
 }
 
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -833,6 +833,10 @@
   llvm::DICompositeType *RetTy = DBuilder.createReplaceableCompositeType(
   getTagForRecord(RD), RDName, Ctx, DefUnit, Line, 0, Size, Align,
   llvm::DINode::FlagFwdDecl, FullName);
+  if (CGM.getCodeGenOpts().DebugFwdTemplateParams)
+if (auto *TSpecial = dyn_cast(RD))
+  DBuilder.replaceArrays(RetTy, llvm::DINodeArray(),
+ CollectCXXTemplateParams(TSpecial, DefUnit));
   ReplaceMap.emplace_back(
   std::piecewise_construct, std::make_tuple(Ty),
   std::make_tuple(static_cast(RetTy)));
Index: include/clang/Frontend/CodeGenOptions.def
===
--- include/clang/Frontend/CodeGenOptions.def
+++ include/clang/Frontend/CodeGenOptions.def
@@ -219,6 +219,10 @@
 CODEGENOPT(SplitDwarfInlining, 1, 1) ///< Whether to include inlining info in 
the
  ///< skeleton CU to allow for 
symbolication
 ///< of inline stack frames without .dwo 
files.
+CODEGENOPT(DebugFwdTemplateParams, 1, 0) ///< Whether to emit complete
+ ///< template parameter descriptions 
in
+ ///< forward declarations (versus just
+ ///< including them in the name).
 
 CODEGENOPT(EmitLLVMUseLists, 1, 0) ///< Control whether to serialize use-lists.
 
Index: include/clang/Driver/CC1Options.td
===
--- include/clang/Driver/CC1Options.td
+++ include/clang/Driver/CC1Options.td
@@ -200,6 +200,9 @@
 def dwarf_ext_refs : Flag<["-"], "dwarf-ext-refs">,
   HelpText<"Generate debug info with external references to clang modules"
" or precompiled headers">;

[PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a reviewer: rnk.
probinson added a comment.

+rnk for the CodeView question.




Comment at: include/clang/Frontend/CodeGenOptions.def:222
 ///< of inline stack frames without .dwo 
files.
+CODEGENOPT(DebugFwdTemplateParams, 1, 0) ///< Whether to emit complete
+ ///< template parameter descriptions 
in

dblaikie wrote:
> Maybe 'Decl' rather than 'Fwd'.
Well, in a sense they are all declarations, and 'Fwd' is a clearer statement of 
the distinction this flag is trying to make.  Unless you feel strongly I'd 
prefer to leave it as is.



Comment at: lib/CodeGen/CGDebugInfo.cpp:836
   llvm::DINode::FlagFwdDecl, FullName);
+  if (CGM.getCodeGenOpts().DebugFwdTemplateParams)
+if (auto *TSpecial = dyn_cast(RD))

It just occurred to me... should CodeView care about this?



Comment at: test/CodeGenCXX/debug-info-fwd-template-param.cpp:7
+template class A;
+A *p;
+

dblaikie wrote:
> Any particular reason for const int, rather than int?
It was the illustrative example of the difference between the demangler ("int 
const") and clang ("const int") that the debugger guys tripped over, and so was 
in the source I started with when creating this test.  I think you are correct, 
it is not important to have it.



https://reviews.llvm.org/D14358



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

[PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I think I will go ahead and commit this; it doesn't change the status quo for 
CodeView and if something is warranted we can do that in a follow-up.


https://reviews.llvm.org/D14358



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

[PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Paul Robinson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL31: [DWARF] Allow forward declarations of a class 
template instantiation (authored by probinson).

Changed prior to commit:
  https://reviews.llvm.org/D14358?vs=116862&id=117030#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D14358

Files:
  cfe/trunk/include/clang/Driver/CC1Options.td
  cfe/trunk/include/clang/Frontend/CodeGenOptions.def
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-fwd-template-param.cpp


Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -833,6 +833,10 @@
   llvm::DICompositeType *RetTy = DBuilder.createReplaceableCompositeType(
   getTagForRecord(RD), RDName, Ctx, DefUnit, Line, 0, Size, Align,
   llvm::DINode::FlagFwdDecl, FullName);
+  if (CGM.getCodeGenOpts().DebugFwdTemplateParams)
+if (auto *TSpecial = dyn_cast(RD))
+  DBuilder.replaceArrays(RetTy, llvm::DINodeArray(),
+ CollectCXXTemplateParams(TSpecial, DefUnit));
   ReplaceMap.emplace_back(
   std::piecewise_construct, std::make_tuple(Ty),
   std::make_tuple(static_cast(RetTy)));
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -2969,6 +2969,11 @@
 CmdArgs.push_back("-generate-type-units");
   }
 
+  // Decide how to render forward declarations of template instantiations.
+  // SCE wants full descriptions, others just get them in the name.
+  if (DebuggerTuning == llvm::DebuggerKind::SCE)
+CmdArgs.push_back("-debug-forward-template-params");
+
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D);
 }
 
Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -528,6 +528,7 @@
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
   Opts.DebugTypeExtRefs = Args.hasArg(OPT_dwarf_ext_refs);
   Opts.DebugExplicitImport = Triple.isPS4CPU();
+  Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
 
   for (const auto &Arg : Args.getAllArgValues(OPT_fdebug_prefix_map_EQ))
 Opts.DebugPrefixMap.insert(StringRef(Arg).split('='));
Index: cfe/trunk/include/clang/Driver/CC1Options.td
===
--- cfe/trunk/include/clang/Driver/CC1Options.td
+++ cfe/trunk/include/clang/Driver/CC1Options.td
@@ -200,6 +200,9 @@
 def dwarf_ext_refs : Flag<["-"], "dwarf-ext-refs">,
   HelpText<"Generate debug info with external references to clang modules"
" or precompiled headers">;
+def debug_forward_template_params : Flag<["-"], 
"debug-forward-template-params">,
+  HelpText<"Emit complete descriptions of template parameters in forward"
+   " declarations">;
 def fforbid_guard_variables : Flag<["-"], "fforbid-guard-variables">,
   HelpText<"Emit an error if a C++ static local initializer would need a guard 
variable">;
 def no_implicit_float : Flag<["-"], "no-implicit-float">,
Index: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
===
--- cfe/trunk/include/clang/Frontend/CodeGenOptions.def
+++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def
@@ -219,6 +219,10 @@
 CODEGENOPT(SplitDwarfInlining, 1, 1) ///< Whether to include inlining info in 
the
  ///< skeleton CU to allow for 
symbolication
 ///< of inline stack frames without .dwo 
files.
+CODEGENOPT(DebugFwdTemplateParams, 1, 0) ///< Whether to emit complete
+ ///< template parameter descriptions 
in
+ ///< forward declarations (versus just
+ ///< including them in the name).
 
 CODEGENOPT(EmitLLVMUseLists, 1, 0) ///< Control whether to serialize use-lists.
 
Index: cfe/trunk/test/CodeGenCXX/debug-info-fwd-template-param.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-fwd-template-param.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-info-fwd-template-param.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -triple=%itanium_abi_triple -debug-info-kind=limited 
-debug-forward-template-params -emit-llvm -o - | FileCheck --check-prefix=CHILD 
%s
+// RUN: %clang_cc1 %s -triple=%itanium_abi_triple -debug-info-kind=limited 
-emit-llvm -o - | FileCheck --check-prefix=NONE %s
+// A DWARF forward declaration of a template instantiati

[PATCH] D35715: Preserve typedef names in debug info for template type parameters

[Paul Robinson via Phabricator via cfe-commits]

probinson abandoned this revision.
probinson added a comment.

Abandoning.  This change is irrelevant to the SCE debugger, and while I believe 
it could be made more complete and better reflect the original source (which is 
what the DWARF spec says names should be), I do not have time to pursue it.

If somebody else feels moved to take it up, go ahead.


https://reviews.llvm.org/D35715



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

[PATCH] D39069: CodeGen: Fix missing debug loc due to alloca

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Anytime the code between saveIP() and restoreIP() could set the current debug 
location, it needs to be saved/restored along with the insertion point.  I have 
to say the problem is not obvious to me here, so maybe saveIP/restoreIP should 
be changed (or eliminated in favor of always using InsertPointGuard).  I'm not 
seeing a lot of places where saveIP/restoreIP are used.

The test looks like all it's doing is verifying both calls have a debug 
location at all.  It could verify that both calls have the _same_ debug 
location, which I would find much more robust and convincing.


https://reviews.llvm.org/D39069



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

[PATCH] D39239: [AST] Incorrectly qualified unscoped enumeration as template actual parameter.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Have you tried this change against the GDB and LLDB test suites?  If they have 
failures then we should think about whether those tests are over-specific or 
whether we should put this under a tuning option.


https://reviews.llvm.org/D39239



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

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

[Paul Robinson via Phabricator via cfe-commits]

probinson added subscribers: cfe-commits, probinson.
probinson added a comment.

+cfe-commits


https://reviews.llvm.org/D51340



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

[PATCH] D46190: For a used declaration, mark any associated usings as referenced.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

@rsmith any further thoughts?  We would really like to get this in before the 
LLVM 7.0 branch, currently scheduled for 1 August.

In https://reviews.llvm.org/D46190#1168027, @CarlosAlbertoEnciso wrote:

> @probinson: I tried `clang-format-diff` and the main format issues are with 
> the test cases.
>
> Do you want the test cases to follow the clang format?


The lib and include files absolutely need to follow clang format.  Tests should 
also, if doing so doesn't disturb their readability.  If clang-format-diff is 
doing things like rearranging comments in the test files, it depends on whether 
it improves or reduces clarity.  Up to you.




Comment at: lib/Sema/SemaDeclCXX.cpp:15515
+
+// Mark the alias declaration and any associated chain as referenced.
+void Sema::MarkNamespaceAliasReferenced(NamedDecl *ND) {

This should use `///` to be  picked up by doxygen.


https://reviews.llvm.org/D46190



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

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Is this because type units depend on COMDAT support?  I had a vague idea that 
COFF also supports COMDAT.


https://reviews.llvm.org/D49594



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

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

[Paul Robinson via Phabricator via cfe-commits]

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

If the plan is for this to be relatively temporary then LGTM.


https://reviews.llvm.org/D49594



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

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

[Paul Robinson via Phabricator via cfe-commits]

probinson added a reviewer: probinson.
probinson added a comment.

Needs a test.


Repository:
  rC Clang

https://reviews.llvm.org/D49652



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

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Also, please document the option in clang/docs/UsersManual.rst.  It should have 
been added when the option was first added, but certainly with right-to-left 
behavior it needs a mention.  Nearly all options work left-to-right, so even 
though it's the same as gcc, not everybody comes to clang from gcc.


Repository:
  rC Clang

https://reviews.llvm.org/D49652



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

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Re. SmallVector versus std::vector, they are functionally similar but have 
different memory-allocation behaviors.  SmallVector includes a vector of N 
elements (where N is the template parameter) so does no dynamic allocation 
until you have more than N elements; but it takes up that much more space as a 
member of a class.  It's mainly intended for stack variables.  Given that this 
particular option is used rarely, I would probably go with std::vector.  The 
code change otherwise is pretty simple and looks fine.

As for testing, it has its quirks for sure.  But I'm happy to help sort out any 
issues.  There should be plenty of CodeGen examples with debug info in the test 
tree to imitate.  You'll want to start with C/C++ source and use `-emit-llvm` 
to get textual IR, then use FileCheck (our fancy-pants grep) to look for the 
new pathname patterns.


Repository:
  rC Clang

https://reviews.llvm.org/D49652



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

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D49652#1172352, @alxu wrote:

> I was just going to run llvm-dwarfdump.


Well, that's one of those quirks I mentioned.  This is a Clang change, and 
Clang tests should exercise as little of LLVM as possible.  That means you 
don't generate assembler or an object file, if you can possibly help it.  And 
in this case, the path renaming should be visible in the debug-info metadata, 
which makes it easy enough to test by emitting LLVM IR and looking at that.  
It's by far the most common test idiom for changes to Clang's CodeGen layer.

Given that the prefix mapping happens in the LLVM IR, it should be no harder to 
emit IR and write a pattern-match than it would be for dwarfdump output.  
Really.  Try `clang -g -S -emit-llvm` on some simple test file, then throw a 
remap option at it and see what's different.


Repository:
  rC Clang

https://reviews.llvm.org/D49652



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

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D49652#1172498, @alxu wrote:

> my general theory is that it's better to have tests that test everything at 
> once if possible, but I guess it's unlikely enough that the debug info path 
> will be correct in the IR and then not correct in the generated file and that 
> that won't be caught by LLVM tests.


The in-tree lit tests are very focused unit tests.  The principle here is that 
a more focused test will be easier to diagnose when it fails.  The LLVM project 
on its own really does not have much end-to-end testing and hardly any of that 
looks at debug info.  Instead we have a bunch of vendors all running their own 
test suites.  It's very much a tradeoff, and I personally view the in-tree lit 
tests as barely more than basic smoke tests.  But it's how the project works, 
and so new in-tree tests need to be consistent with that approach.


Repository:
  rC Clang

https://reviews.llvm.org/D49652



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

[PATCH] D46190: For a used declaration, mark any associated usings as referenced.

[Paul Robinson via Phabricator via cfe-commits]

probinson accepted this revision.
probinson added a comment.

Although I am far from expert in how Clang manages declarations, AFAICT this 
does what @rsmith requested, so LGTM.


https://reviews.llvm.org/D46190



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

[PATCH] D49953: [compiler-rt] Add a routine to specify the mode used when creating profile dirs.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Is it possible/reasonable for the test to call __llvm_profile_recursive_mkdir 
and then verify the mode of the created directory?
The test would have to be flagged as unsupported on Windows but that seems fine.


https://reviews.llvm.org/D49953



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

[PATCH] D49953: [compiler-rt] Add a routine to specify the mode used when creating profile dirs.

[Paul Robinson via Phabricator via cfe-commits]

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

Using `%t` with suffixes instead of `%T` is the preferred idiom, glad you 
worked that out.
The more elaborate test depends on a set of Posix-y calls, but that's fine.  If 
there are more environments that can't handle it, the bots will find them for 
you. :-)
LGTM with the fancier test, once you fix the missing RUN.  (The RUN prefixes 
are how lit decides which lines are the test script.)




Comment at: test/profile/instrprof-set-dir-mode2.c:3
+// RUN: %clang_pgogen -o %t.bin %s -DTESTPATH=\"%t.dir\"
+// rm -rf %t.dir
+// RUN: %run %t.bin

This needs a RUN: in front of it, I think.


https://reviews.llvm.org/D49953



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

[PATCH] D43189: [DebugInfo] Avoid name conflict of generated VLA expression variable.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Please make sure to cite the PR in the commit message.


https://reviews.llvm.org/D43189



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

[PATCH] D42766: [DebugInfo] Support DWARFv5 source code embedding extension

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Looks generally straightforward.  I'd give other people a chance to chime in 
but I have only minor comments.




Comment at: lib/CodeGen/CGDebugInfo.cpp:411
 // If the location is not valid then use main input file.
-return DBuilder.createFile(remapDIPath(TheCU->getFilename()),
-   remapDIPath(TheCU->getDirectory()),
-   TheCU->getFile()->getChecksumKind(),
-   TheCU->getFile()->getChecksum());
+return getOrCreateMainFile();
 

Looks like this simplification (replace two calls to createFile() with 
getOrCreateMainFile()) could be its own NFC commit and simplify this patch 
slightly.



Comment at: lib/Driver/ToolChains/Clang.cpp:3021
+  if (Args.hasFlag(options::OPT_gembed_source, options::OPT_gno_embed_source, 
false)) {
+// Source embedding is a DWARFv5 extension. By now we have checked if a
+// DWARF version was stated explicitly, and have otherwise fallen back to

Maybe "is an extension to DWARF v5" to clarify it's not actually part of DWARF 
v5.


Repository:
  rC Clang

https://reviews.llvm.org/D42766



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

[PATCH] D43700: Emit proper CodeView even when not using the cl driver.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Being a cross-compiler I think it's generally a good thing to have more 
combinations be less broken.
Note that PS4's compiler is hosted on Windows and uses the gcc-style driver; 
it's convenient sometimes to be able to target Windows without having to learn 
a new driver language.  So thanks for doing this!


https://reviews.llvm.org/D43700



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

[PATCH] D39239: [AST] Incorrectly qualified unscoped enumeration as template actual parameter.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a project: debug-info.
probinson accepted this revision.
probinson added a comment.
This revision is now accepted and ready to land.

With the GDB test results and LLDB able to handle it, this LGTM.
Carlos, do you have commit access?


https://reviews.llvm.org/D39239



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

[PATCH] D39239: [AST] Incorrectly qualified unscoped enumeration as template actual parameter.

[Paul Robinson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC321312: [AST] Incorrectly qualified unscoped enumeration as 
template actual parameter. (authored by probinson, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D39239?vs=120066&id=127937#toc

Repository:
  rC Clang

https://reviews.llvm.org/D39239

Files:
  lib/AST/Decl.cpp
  test/Modules/odr.cpp
  test/SemaCXX/return-noreturn.cpp
  test/SemaTemplate/temp_arg_enum_printing.cpp
  test/SemaTemplate/temp_arg_enum_printing_more.cpp
  unittests/AST/NamedDeclPrinterTest.cpp

Index: unittests/AST/NamedDeclPrinterTest.cpp
===
--- unittests/AST/NamedDeclPrinterTest.cpp
+++ unittests/AST/NamedDeclPrinterTest.cpp
@@ -143,7 +143,7 @@
   ASSERT_TRUE(PrintedWrittenNamedDeclCXX11Matches(
 "enum X { A };",
 "A",
-"X::A"));
+"A"));
 }
 
 TEST(NamedDeclPrinter, TestScopedNamedEnum) {
@@ -164,7 +164,7 @@
   ASSERT_TRUE(PrintedWrittenNamedDeclCXX11Matches(
 "class X { enum Y { A }; };",
 "A",
-"X::Y::A"));
+"X::A"));
 }
 
 TEST(NamedDeclPrinter, TestClassWithScopedNamedEnum) {
Index: test/SemaTemplate/temp_arg_enum_printing_more.cpp
===
--- test/SemaTemplate/temp_arg_enum_printing_more.cpp
+++ test/SemaTemplate/temp_arg_enum_printing_more.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fsyntax-only -ast-print %s -std=c++11 | FileCheck %s
+
+// Make sure that for template value arguments that are unscoped enumerators,
+// no qualified enum information is included in their name, as their visibility
+// is global. In the case of scoped enumerators, they must include information
+// about their enum enclosing scope.
+
+enum E1 { e1 };
+template struct tmpl_1 {};
+// CHECK: template<> struct tmpl_1
+tmpl_1 TMPL_1;  // Name must be 'e1'.
+
+namespace nsp_1 { enum E2 { e2 }; }
+template struct tmpl_2 {};
+// CHECK: template<> struct tmpl_2
+tmpl_2 TMPL_2;   // Name must be 'nsp_1::e2'.
+
+enum class E3 { e3 };
+template struct tmpl_3 {};
+// CHECK: template<> struct tmpl_3
+tmpl_3 TMPL_3;  // Name must be 'E3::e3'.
+
+namespace nsp_2 { enum class E4 { e4 }; }
+template struct tmpl_4 {};
+// CHECK: template<> struct tmpl_4
+tmpl_4 TMPL_4;   // Name must be 'nsp_2::E4::e4'.
Index: test/SemaTemplate/temp_arg_enum_printing.cpp
===
--- test/SemaTemplate/temp_arg_enum_printing.cpp
+++ test/SemaTemplate/temp_arg_enum_printing.cpp
@@ -13,9 +13,9 @@
 void foo();
   
 void test() {
-  // CHECK: template<> void foo()
+  // CHECK: template<> void foo()
   NamedEnumNS::foo();
-  // CHECK: template<> void foo()
+  // CHECK: template<> void foo()
   NamedEnumNS::foo<(NamedEnum)1>();
   // CHECK: template<> void foo<2>()
   NamedEnumNS::foo<(NamedEnum)2>();
Index: test/SemaCXX/return-noreturn.cpp
===
--- test/SemaCXX/return-noreturn.cpp
+++ test/SemaCXX/return-noreturn.cpp
@@ -143,7 +143,7 @@
 } // expected-warning {{control reaches end of non-void function}}
 
 void PR9412_f() {
-PR9412_t(); // expected-note {{in instantiation of function template specialization 'PR9412_t' requested here}}
+PR9412_t(); // expected-note {{in instantiation of function template specialization 'PR9412_t' requested here}}
 }
 
 struct NoReturn {
Index: test/Modules/odr.cpp
===
--- test/Modules/odr.cpp
+++ test/Modules/odr.cpp
@@ -18,6 +18,6 @@
 // expected-note@a.h:3 {{declaration of 'f' does not match}}
 // expected-note@a.h:1 {{definition has no member 'm'}}
 
-// expected-error@b.h:5 {{'E::e2' from module 'b' is not present in definition of 'E' in module 'a'}}
+// expected-error@b.h:5 {{'e2' from module 'b' is not present in definition of 'E' in module 'a'}}
 // expected-error@b.h:3 {{'Y::f' from module 'b' is not present in definition of 'Y' in module 'a'}}
 // expected-error@b.h:2 {{'Y::m' from module 'b' is not present in definition of 'Y' in module 'a'}}
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -1548,7 +1548,10 @@
   // enumerator is declared in the scope that immediately contains
   // the enum-specifier. Each scoped enumerator is declared in the
   // scope of the enumeration.
-  if (ED->isScoped() || ED->getIdentifier())
+  // For the case of unscoped enumerator, do not include in the qualified
+  // name any information about its enum enclosing scope, as is visibility
+  // is global.
+  if (ED->isScoped())
 OS << *ED;
   else
 continue;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D41421: Eliminate a magic number. NFC.

[Paul Robinson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC321757: Calculate size of buffer instead of using a magic 
value. (authored by probinson, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D41421

Files:
  lib/Frontend/PrintPreprocessedOutput.cpp


Index: lib/Frontend/PrintPreprocessedOutput.cpp
===
--- lib/Frontend/PrintPreprocessedOutput.cpp
+++ lib/Frontend/PrintPreprocessedOutput.cpp
@@ -752,7 +752,7 @@
 } else if (Tok.isLiteral() && !Tok.needsCleaning() &&
Tok.getLiteralData()) {
   OS.write(Tok.getLiteralData(), Tok.getLength());
-} else if (Tok.getLength() < 256) {
+} else if (Tok.getLength() < llvm::array_lengthof(Buffer)) {
   const char *TokPtr = Buffer;
   unsigned Len = PP.getSpelling(Tok, TokPtr);
   OS.write(TokPtr, Len);


Index: lib/Frontend/PrintPreprocessedOutput.cpp
===
--- lib/Frontend/PrintPreprocessedOutput.cpp
+++ lib/Frontend/PrintPreprocessedOutput.cpp
@@ -752,7 +752,7 @@
 } else if (Tok.isLiteral() && !Tok.needsCleaning() &&
Tok.getLiteralData()) {
   OS.write(Tok.getLiteralData(), Tok.getLength());
-} else if (Tok.getLength() < 256) {
+} else if (Tok.getLength() < llvm::array_lengthof(Buffer)) {
   const char *TokPtr = Buffer;
   unsigned Len = PP.getSpelling(Tok, TokPtr);
   OS.write(TokPtr, Len);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47291: Proposal to make rtti errors more generic.

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6729
 def err_no_dynamic_cast_with_fno_rtti : Error<
-  "cannot use dynamic_cast with -fno-rtti">;
+  "use of dynamic_cast requires enabling RTTI">;
 

wristow wrote:
> Sunil_Srivastava wrote:
> > probinson wrote:
> > > filcab wrote:
> > > > I'd prefer to have the way to enable RTTI mentioned in the message. 
> > > > Could we have something like `ToolChain::getRTTIMode()` and have a 
> > > > "RTTI was on/of" or "RTTI defaulted to on/off"? That way we'd be able 
> > > > to have a message similar to the current one (mentioning "you passed 
> > > > -fno-rtti") on platforms that default to RTTI=on, and have your updated 
> > > > message (possibly with a mention of "use -frtti") on platforms that 
> > > > default to RTTI=off.
> > > > 
> > > > (This is a minor usability comment about this patch, I don't consider 
> > > > it a blocker or anything)
> > > If the options are spelled differently for clang-cl and we had a way to 
> > > retrieve the appropriate spellings, providing the option to use in the 
> > > diagnostic does seem like a nice touch.
> > The idea of suggestion as to how-to-turn-on-rtti is appealing, but it is 
> > not trivial.
> > 
> > First, clang-cl does not give this warning at all, so the issue is moot for 
> > clang-cl.
> > 
> > For unix-line command-line, if the default RTTI mode in ENABLED (the 
> > unknown-linux)
> > then this warning appears only if the user gives -fno-rtti options, so 
> > again we do
> > not need to say anything more.
> > 
> > The only cases left are compilers a where the default RTTI mode is 
> > DISABLED. 
> > Here an addendum like '[-frtti]' may make sense. AFAIK, PS4 is the only 
> > compiler
> > with this default, but there may be other such private compilers.
> > 
> > So should we append '[-frtti]' if Target.getTriple().isPS4() is true?
> > So should we append '[-frtti]' if Target.getTriple().isPS4() is true?
> 
> Personally, I'd be OK with producing a suggestion of how to enable RTTI based 
> on the PS4 triple.  But I'd also be OK with not enhancing this diagnostic to 
> suggest how to turn on RTTI (that is, going with the patch as originally 
> proposed here).
> 
> If clang-cl produced a warning when a dynamic_cast or typeid construct was 
> encountered in `/GR-` mode, then I'd feel it's worth complicating the code to 
> provide a target-sensitive way for advising the user how to turn RTTI on.  
> But clang-cl doesn't produce a warning in that case, so the effort to add the 
> framework for producing a target-sensitive warning doesn't seem worth it to 
> me.
> 
> Improving clang-cl to produce a diagnostic in this `/GR-` situation seems 
> like a good idea, but separate from this proposed patch.  If that work gets 
> done at some point, then it would be natural to revisit this diagnostic at 
> that time.
If clang-cl is not a consideration, then I think the easiest and clearest way 
to do this is simply to say `requires -frtti` without hair-splitting which 
targets default which way.


Repository:
  rC Clang

https://reviews.llvm.org/D47291



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

[PATCH] D47291: Proposal to make rtti errors more generic.

[Paul Robinson via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D47291



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

[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

@rsmith anything else needed here?


https://reviews.llvm.org/D46190



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

[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

@dblaikie is @rsmith back from the standards meeting yet? I hate to be a pest 
but this is blocking other work Carlos has in progress.


https://reviews.llvm.org/D46190



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

[PATCH] D46190: For a referenced declaration, mark any associated usings as referenced.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Style comments.
The two new Sema methods (for namespaces and using references) are C++ 
specific, so SemaDeclCXX.cpp would seem like a more appropriate home for them.




Comment at: lib/Sema/Sema.cpp:1879
+void Sema::MarkNamespaceAliasReferenced(NamedDecl *ND) {
+  if (ND && !ND->isReferenced()) {
+NamespaceAliasDecl *NA = nullptr;

You could do this as an early return and reduce indentation in the rest of the 
method.
```
if (!ND || ND->isReferenced())
  return;
```




Comment at: lib/Sema/Sema.cpp:1880
+  if (ND && !ND->isReferenced()) {
+NamespaceAliasDecl *NA = nullptr;
+while ((NA = dyn_cast(ND)) && !NA->isReferenced()) {

Initializing this to nullptr is redundant, as you set NA in the while-loop 
expression.



Comment at: lib/Sema/Sema.cpp:1891
+/// \brief Mark as referenced any 'using declaration' that have introduced
+/// the given declaration in the current context.
+void Sema::MarkUsingReferenced(Decl *D, CXXScopeSpec *SS, Expr *E) {

`\brief` is unnecessary, as we have auto-brief turned on.



Comment at: lib/Sema/Sema.cpp:1903
+  if (auto *NNS = SS ? SS->getScopeRep()
+ : E ? dyn_cast(E)->getQualifier()
+ : nullptr) {

This dyn_cast<> can be simply a cast<>.



Comment at: lib/Sema/Sema.cpp:1916
+  if ((USD = dyn_cast(DR)) && !USD->isReferenced()) {
+if (USD->getTargetDecl() == D) {
+  USD->setReferenced();

You could sink the declaration of USD like so:
```
for (auto *DR : S->decls())
  if (auto *USD = dyn_cast(DR))
if (!USD->isReferenced() && USD->getTargetDecl() == D) {
```
Also I would put braces around the 'for' loop body, even though it is 
technically one statement.



Comment at: lib/Sema/Sema.cpp:1927
+// Check if the declaration was introduced by a 'using-directive'.
+auto *Target = dyn_cast(DC);
+for (auto *UD : S->using_directives())

You verified that his is a namespace earlier, so use cast<> not dyn_cast<>.


https://reviews.llvm.org/D46190



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

[PATCH] D47720: [DebugInfo] Inline for without DebugLocation

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D47720#1136585, @gramanas wrote:

> ping! should I commit this?


@vsk accepted it, so yes go ahead.


Repository:
  rC Clang

https://reviews.llvm.org/D47720



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

[PATCH] D57162: [DEBUG_INFO][NVPTX] Generate correct data about variable address class.

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4235
 CGM.getContext().getTargetAddressSpace(D->getType());
+if (CGM.getLangOpts().CUDA && CGM.getLangOpts().CUDAIsDevice) {
+  if (D->hasAttr())

Can a variable have one of these CUDA attributes when CUDAIsDevice is false? 
I'm just wondering if the extra level of checking is really necessary or useful.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57162/new/

https://reviews.llvm.org/D57162



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D52296#1282458, @dblaikie wrote:

> I guess in that case your distributed build system would have a constraint 
> that it always ships all the object files back to the primary machine (where 
> you'd be running the debugger)? (perhaps it just always runs the link locally 
> - even though it distributes the compilations - I guess that's probably how 
> things like distcc work, where they only inject themselves into the 
> compilation command, not the linking command maybe?)


AFAIK this is how distcc, icecc, SN-DBS all work.  Compilations are 
distributed, the .o files come back, links are local.

> Also, may require some work/more flags to handle the possible file renaming 
> (or relative/absolute adjustment) when object files are built on a remote 
> system in one location, but then moved back to the local system and placed in 
> another location?

Any distributed build has to make this work, so the paths in the line table are 
usable.  Not clear what you're thinking might be the problem with the 
split-in-.o case.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D52296#1282589, @dblaikie wrote:

> In https://reviews.llvm.org/D52296#1282587, @probinson wrote:
>
> > Any distributed build has to make this work, so the paths in the line table 
> > are usable.  Not clear what you're thinking might be the problem with the 
> > split-in-.o case.
>
>
> Fair point - same sort of problem, but might need distinct handling (ie: 
> might not work "for free" with existing tools, but need more support), etc, 
> depending on how it's solved?
>
> Mostly just wondering whether Clang would need extra flags to support this - 
> to get a sense of the total/overall solution/surface area of the feature, etc.


Shouldn't.  We have licensees using distributed builds all day every day, but 
we haven't introduced anything into the compiler itself to make that work.  On 
Windows, I know SN-DBS will intercept system calls to patch up filespecs.  On 
Linux I'd expect the remote servers to set up chroot environments so the path 
names will Just Work, although I admit I've never actually looked.  Never had 
to; it Just Works.
In this patch, where the .o normally points to the .dwo it would instead just 
point to itself, right?  The linker doesn't need to fix anything up, it just 
ignores the .dwo sections.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D52296#1283691, @grimar wrote:

> Nice :) 
>  So seems the last unresolved question left is the naming of the new option?
>  If we do not want to go with `-gsingle-file-split-dwarf` then what it should 
> be?
>
> What about `-fdebug-fission` as an alias for `-gsplit-dwarf`.
>  And `-fsingle-file-debug-fission` for the new option?
>
> Or, may be:
>
> `-fdebug-fission` for the new option and then
>  `-fdebug-fission -fdebug-split-dwo` would work together as `-gsplit-dwarf`?


Only DWARF supports this, correct?  So I would suggest: 
`-fdwarf-fission[={split,single}]` where the parameter defaults to `split`. Is 
there any particular value in having two separate options?


https://reviews.llvm.org/D52296



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

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

[Paul Robinson via Phabricator via cfe-commits]

probinson added subscribers: gbedwell, probinson.
probinson added a comment.

+gbedwell

Just to throw the idea out there, why not abandon debuginfo-tests entirely and 
try using Dexter for this. Dexter's design center is debug-info quality, not 
correctness, but it already knows how to drive several different debuggers on 
multiple platforms.
Dexter would have to become an LLVM project tool, and I'm not sure how Sony 
management feels about that, but I think this would be an awesome use-case.


https://reviews.llvm.org/D54187



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

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D54187#1290294, @zturner wrote:

> In https://reviews.llvm.org/D54187#1290282, @probinson wrote:
>
> > +gbedwell
> >
> > Just to throw the idea out there, why not abandon debuginfo-tests entirely 
> > and try using Dexter for this. Dexter's design center is debug-info 
> > quality, not correctness, but it already knows how to drive several 
> > different debuggers on multiple platforms.
> >  Dexter would have to become an LLVM project tool, and I'm not sure how 
> > Sony management feels about that, but I think this would be an awesome 
> > use-case.
>
>
> Depends where we draw the distinction between quality and correctness.  We 
> specifically want a way to test that when we fix a correctness bug, it's 
> actually fixed against Microsoft debuggers.


Dexter knows how to drive Visual Studio tools already, as well as gdb and 
(maybe) lldb.  I have never looked inside it but I'd expect Greg to have made 
it straightforward to add new tools.


https://reviews.llvm.org/D54187



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

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D54187#1290340, @gbedwell wrote:

> (Dexter) will step through every line of the program it can, collecting info 
> about each step until it reaches the program exit.  It won't currently 
> produce a pass/fail, but rather a score.  That is, if it observes that 'x' is 
> visible and has the value 42 it'll get a perfect score.


Seems like it would easy enough to base pass/fail on getting a perfect score, 
for the purpose of debuginfo-tests.


https://reviews.llvm.org/D54187



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

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:5889
   const llvm::Triple &T = getToolChain().getTriple();
-  if (Args.hasArg(options::OPT_gsplit_dwarf) &&
+  if ((getDebugFissionKind(D, Args) == DwarfFissionKind::Split) &&
   (T.isOSLinux() || T.isOSFuchsia())) {

grimar wrote:
> dblaikie wrote:
> > Could having more than one call to getDebugFissionKind lead to the error 
> > message (for -fdwarf-fission=garbage) being printed multiple times? Or is 
> > this call on a distinct/mutually exclusive codepath to the other?
> I think it should not be possible. Currently, there are 2 inclusions of the 
> `getDebugFissionKind `. One is used for construction clang job,
> and one is here, it is used to construct an assembler tool job. I do not see 
> a way how it can be printed multiple times.
> 
> For example, the following invocation will print it only once:
> 
> ```
> clang main.c -o out.s -S -fdwarf-fission=foo
> clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> ```
The example you give here will not create an assembler job. Try
`clang main.c -fdwarf-fission=foo -fno-integrated-as` which I think will 
exercise the path David is asking about.


https://reviews.llvm.org/D52296



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

[PATCH] D54756: [DebugInfo] NFC Clang test changes for DISubprogram flags

[Paul Robinson via Phabricator via cfe-commits]

probinson created this revision.
probinson added reviewers: dblaikie, aprantl.
probinson added a project: debug-info.
Herald added a subscriber: eraman.

See https://reviews.llvm.org/D54755


Repository:
  rC Clang

https://reviews.llvm.org/D54756

Files:
  clang/test/CodeGen/debug-info-scope-file.c
  clang/test/CodeGenCXX/PR20038.cpp
  clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp
  clang/test/CodeGenCXX/debug-info-access.cpp
  clang/test/CodeGenCXX/debug-info-blocks.cpp
  clang/test/CodeGenCXX/debug-info-cxx1y.cpp
  clang/test/CodeGenCXX/debug-info-decl-nested.cpp
  clang/test/CodeGenCXX/debug-info-function-context.cpp
  clang/test/CodeGenCXX/debug-info-global-ctor-dtor.cpp
  clang/test/CodeGenCXX/debug-info-inlined.cpp
  clang/test/CodeGenCXX/debug-info-ms-abi.cpp
  clang/test/CodeGenCXX/debug-info-namespace.cpp
  clang/test/CodeGenCXX/debug-info-static-fns.cpp
  clang/test/CodeGenCXX/debug-info-thunk-msabi.cpp
  clang/test/CodeGenCXX/debug-info-thunk.cpp
  clang/test/CodeGenCXX/debug-info.cpp
  clang/test/CodeGenCXX/debug-lambda-expressions.cpp
  clang/test/CodeGenCXX/globalinit-loc.cpp
  clang/test/CodeGenCXX/linetable-fnbegin.cpp
  clang/test/CodeGenObjC/arc-linetable.m
  clang/test/CodeGenObjC/debug-info-category.m
  clang/test/CodeGenObjC/debug-info-synthesis.m
  clang/test/CodeGenObjC/debug-property-synth.m
  clang/test/CodeGenObjC/debuginfo-properties.m

Index: clang/test/CodeGenObjC/debuginfo-properties.m
===
--- clang/test/CodeGenObjC/debuginfo-properties.m
+++ clang/test/CodeGenObjC/debuginfo-properties.m
@@ -13,16 +13,16 @@
 @property (nonatomic, retain) Selection* selection;
 // CHECK: !DISubprogram(name: "-[MyClass selection]"
 // CHECK-SAME:  line: [[@LINE-2]]
-// CHECK-SAME:  isLocal: true, isDefinition: true
+// CHECK-SAME:  DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK: !DISubprogram(name: "-[MyClass setSelection:]"
 // CHECK-SAME:  line: [[@LINE-5]]
-// CHECK-SAME:  isLocal: true, isDefinition: true
+// CHECK-SAME:  DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK: !DISubprogram(name: "-[OtherClass selection]"
 // CHECK-SAME:  line: [[@LINE-8]]
-// CHECK-SAME:  isLocal: true, isDefinition: true
+// CHECK-SAME:  DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK: !DISubprogram(name: "-[OtherClass setSelection:]"
 // CHECK-SAME:  line: [[@LINE-11]]
-// CHECK-SAME:  isLocal: true, isDefinition: true
+// CHECK-SAME:  DISPFlagLocalToUnit | DISPFlagDefinition
 
 @end
 
Index: clang/test/CodeGenObjC/debug-property-synth.m
===
--- clang/test/CodeGenObjC/debug-property-synth.m
+++ clang/test/CodeGenObjC/debug-property-synth.m
@@ -18,9 +18,9 @@
 // CHECK-NOT: ret
 // CHECK: load {{.*}}, !dbg ![[DBG2:[0-9]+]]
 //
-// CHECK: !DISubprogram(name: "-[I p1]",{{.*}} line: [[@LINE+4]],{{.*}} isLocal: true, isDefinition: true
+// CHECK: !DISubprogram(name: "-[I p1]",{{.*}} line: [[@LINE+4]],{{.*}} DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK: ![[DBG1]] = !DILocation(line: [[@LINE+3]],
-// CHECK: !DISubprogram(name: "-[I setP1:]",{{.*}} line: [[@LINE+2]],{{.*}} isLocal: true, isDefinition: true
+// CHECK: !DISubprogram(name: "-[I setP1:]",{{.*}} line: [[@LINE+2]],{{.*}} DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK: ![[DBG2]] = !DILocation(line: [[@LINE+1]],
 @property int p1;
 @end
Index: clang/test/CodeGenObjC/debug-info-synthesis.m
===
--- clang/test/CodeGenObjC/debug-info-synthesis.m
+++ clang/test/CodeGenObjC/debug-info-synthesis.m
@@ -34,4 +34,4 @@
 // CHECK: !DISubprogram(name: "-[Foo setDict:]"
 // CHECK-SAME:  file: ![[FILE]],
 // CHECK-SAME:  line: 8,
-// CHECK-SAME:  isLocal: true, isDefinition: true
+// CHECK-SAME:  DISPFlagLocalToUnit | DISPFlagDefinition
Index: clang/test/CodeGenObjC/debug-info-category.m
===
--- clang/test/CodeGenObjC/debug-info-category.m
+++ clang/test/CodeGenObjC/debug-info-category.m
@@ -36,17 +36,18 @@
 
 // CHECK: ![[STRUCT:.*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
 
-// DWARF5: !DISubprogram(name: "-[Foo integer]", scope: ![[STRUCT]], {{.*}}isDefinition: false
-// DWARF5: !DISubprogram(name: "-[Foo integer:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
-// DWARF5: !DISubprogram(name: "+[Foo(Bar) zero:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
-// DWARF5: !DISubprogram(name: "-[Foo(Bar) add:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
-
-// DWARF4-NOT: !DISubprogram(name: "-[Foo integer]", scope: ![[STRUCT]], {{.*}}isDefinition: false
-// DWARF4-NOT: !DISubprogram(name: "-[Foo integer:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
-// DWARF4-NOT: !DISubprogram(name: "+[Foo(Bar) zero:]", scope: ![[STRUCT]], {{.*}}isDefinition:

[PATCH] D54756: [DebugInfo] NFC Clang test changes for DISubprogram flags

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Ping. @aprantl approved the dependent patch D54755 
.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54756/new/

https://reviews.llvm.org/D54756



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

[PATCH] D54756: [DebugInfo] NFC Clang test changes for DISubprogram flags

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D54756#1311679 , @aprantl wrote:

> Everybody with out-of-tree tests will be excited ;-)


Yeah... fortunately we still accept the old syntax on input, so it's only 
people with checks on DISubprogram *output* that will need to update.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54756/new/

https://reviews.llvm.org/D54756



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

[PATCH] D54756: [DebugInfo] NFC Clang test changes for DISubprogram flags

[Paul Robinson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL347807: [DebugInfo] NFC Clang test changes for: IR/Bitcode 
changes for DISubprogram… (authored by probinson, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54756?vs=174774&id=175756#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54756/new/

https://reviews.llvm.org/D54756

Files:
  cfe/trunk/test/CodeGen/debug-info-scope-file.c
  cfe/trunk/test/CodeGenCXX/PR20038.cpp
  cfe/trunk/test/CodeGenCXX/dbg-info-all-calls-described.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-access.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-blocks.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-cxx1y.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-decl-nested.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-function-context.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-global-ctor-dtor.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-inlined.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-ms-abi.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-namespace.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-static-fns.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-thunk-msabi.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-thunk.cpp
  cfe/trunk/test/CodeGenCXX/debug-info.cpp
  cfe/trunk/test/CodeGenCXX/debug-lambda-expressions.cpp
  cfe/trunk/test/CodeGenCXX/globalinit-loc.cpp
  cfe/trunk/test/CodeGenCXX/linetable-fnbegin.cpp
  cfe/trunk/test/CodeGenObjC/arc-linetable.m
  cfe/trunk/test/CodeGenObjC/debug-info-category.m
  cfe/trunk/test/CodeGenObjC/debug-info-synthesis.m
  cfe/trunk/test/CodeGenObjC/debug-property-synth.m
  cfe/trunk/test/CodeGenObjC/debuginfo-properties.m

Index: cfe/trunk/test/CodeGenCXX/debug-info-blocks.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-blocks.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-info-blocks.cpp
@@ -14,7 +14,7 @@
 
 // CHECK: !DISubprogram(name: "__Block_byref_object_copy_",
 // CHECK-SAME:  line: 11,
-// CHECK-SAME:  isLocal: true, isDefinition: true
+// CHECK-SAME:  DISPFlagLocalToUnit | DISPFlagDefinition
 // CHECK: !DISubprogram(name: "__Block_byref_object_dispose_",
 // CHECK-SAME:  line: 11,
-// CHECK-SAME:  isLocal: true, isDefinition: true
+// CHECK-SAME:  DISPFlagLocalToUnit | DISPFlagDefinition
Index: cfe/trunk/test/CodeGenCXX/PR20038.cpp
===
--- cfe/trunk/test/CodeGenCXX/PR20038.cpp
+++ cfe/trunk/test/CodeGenCXX/PR20038.cpp
@@ -6,9 +6,9 @@
 extern bool b;
 // CHECK: call {{.*}}, !dbg [[DTOR_CALL1_LOC:![0-9]*]]
 // CHECK: call {{.*}}, !dbg [[DTOR_CALL2_LOC:![0-9]*]]
-// CHECK: [[FUN1:.*]] = distinct !DISubprogram(name: "fun1",{{.*}} isDefinition: true
+// CHECK: [[FUN1:.*]] = distinct !DISubprogram(name: "fun1",{{.*}} DISPFlagDefinition
 // CHECK: [[DTOR_CALL1_LOC]] = !DILocation(line: [[@LINE+1]], scope: [[FUN1]])
 void fun1() { b && (C(), 1); }
-// CHECK: [[FUN2:.*]] = distinct !DISubprogram(name: "fun2",{{.*}} isDefinition: true
+// CHECK: [[FUN2:.*]] = distinct !DISubprogram(name: "fun2",{{.*}} DISPFlagDefinition
 // CHECK: [[DTOR_CALL2_LOC]] = !DILocation(line: [[@LINE+1]], scope: [[FUN2]])
 bool fun2() { return (C(), b) && 0; }
Index: cfe/trunk/test/CodeGenCXX/linetable-fnbegin.cpp
===
--- cfe/trunk/test/CodeGenCXX/linetable-fnbegin.cpp
+++ cfe/trunk/test/CodeGenCXX/linetable-fnbegin.cpp
@@ -7,7 +7,7 @@
 // CHECK: [[HPP:.*]] = !DIFile(filename: "./template.hpp",
 // CHECK: [[SP:.*]] = distinct !DISubprogram(name: "bar",
 // CHECK-SAME:   file: [[HPP]], line: 22
-// CHECK-SAME:   isDefinition: true
+// CHECK-SAME:   DISPFlagDefinition
 // We shouldn't need a lexical block for this function.
 // CHECK: [[DBG]] = !DILocation(line: 23, scope: [[SP]])
 
Index: cfe/trunk/test/CodeGenCXX/debug-lambda-expressions.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-lambda-expressions.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-lambda-expressions.cpp
@@ -40,7 +40,7 @@
 // CHECK: ![[INT:[0-9]+]] = !DIBasicType(name: "int"
 
 // A: 10
-// CHECK: ![[A_FUNC:.*]] = distinct !DISubprogram(name: "a"{{.*}}, line: [[A_LINE:[0-9]+]]{{.*}}, isDefinition: true
+// CHECK: ![[A_FUNC:.*]] = distinct !DISubprogram(name: "a"{{.*}}, line: [[A_LINE:[0-9]+]]{{.*}} DISPFlagDefinition
 
 // Back to A. -- 78
 // CHECK: ![[LAM_A:.*]] = distinct !DICompositeType(tag: DW_TAG_class_type{{.*}}, scope: ![[A_FUNC]]{{.*}}, line: [[A_LINE]],
@@ -52,7 +52,7 @@
 // CHECK-SAME:   DIFlagPublic
 
 // B: 14
-// CHECK: ![[B_FUNC:.*]] = distinct !DISubprogram(name: "b"{{.*}}, line: [[B_LINE:[0-9]+]]{{.*}}, isDefinition: true
+// CHECK: ![[B_FUNC:.*]] = distinct !DISubprogram(name: "b"{{.*}}, line: [[B_

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

> When building the FileCheck binary with debug info, this patch makes the 
> build artifacts ~1kb smaller.

Nice!




Comment at: lib/IR/DiagnosticInfo.cpp:39
 #include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/raw_ostream.h"
 #include 

Do we use a case-sensitive sort of include files?  I thought it was 
insensitive.  Of course the coding standard doesn't say. :-P


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55085/new/

https://reviews.llvm.org/D55085



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

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: lib/IR/DiagnosticInfo.cpp:39
 #include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/raw_ostream.h"
 #include 

aprantl wrote:
> probinson wrote:
> > Do we use a case-sensitive sort of include files?  I thought it was 
> > insensitive.  Of course the coding standard doesn't say. :-P
> I just ran clang-format without thinking about this...
"Don't argue with clang-format."  Okay.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55085/new/

https://reviews.llvm.org/D55085



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

[PATCH] D55137: Honor -fdebug-prefix-map when creating function names for the debug info.

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:238
+  // Apply -fdebug-prefix-map.
+  PP.RemapFilePaths = true;
+  PP.remapPath = [this](StringRef Path) { return remapDIPath(Path); };

Unconditionally?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55137/new/

https://reviews.llvm.org/D55137



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

[PATCH] D55137: Honor -fdebug-prefix-map when creating function names for the debug info.

[Paul Robinson via Phabricator via cfe-commits]

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

LGTM aside from a grump about the test, which is totally up to you.




Comment at: test/CodeGenCXX/debug-prefix-map-lambda.cpp:7
+  // CHECK: !DISubprogram(name: "b<(lambda at
+  // CHECK-SAME:  /SOURCE_ROOT/debug-prefix-map-lambda.cpp
+  // CHECK-SAME:  [[@LINE+1]]:{{[0-9]+}})>"

I've never liked tests that hard-code the test file name. FileCheck lets you 
define variables on the RUN line, is there a lit substitution that will return 
just the basename?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55137/new/

https://reviews.llvm.org/D55137



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

[PATCH] D15881: [DWARF] Omitting the explicit import of an anonymous namespace is a debugger-tuning decision, not a target decision.

[Paul Robinson via Phabricator via cfe-commits]

probinson abandoned this revision.
probinson added a comment.
Herald added a subscriber: JDevlieghere.

Abandoning dead patch.  This wound up being done a different way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D15881/new/

https://reviews.llvm.org/D15881



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

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I'd preserve the trivial test cases and show that the NonTrivial flag is *not* 
present.  I can suggest ways to achieve this if you like (note there is no 
CHECK-DAG-NOT combined directive).




Comment at: test/CodeGenCXX/debug-info-composite-triviality.cpp:88
-
-// CHECK-DAG: !DICompositeType({{.*}}, name: "NonTrivialE",{{.*}}flags: 
{{.*}}DIFlagNonTrivial
-struct NonTrivialE : Trivial, NonTrivial {

This one used to be flagged nontrivial, and now it's not flagged?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59347/new/

https://reviews.llvm.org/D59347



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

[PATCH] D59815: [Driver] Enable -fsanitize-address-globals-dead-stripping by default on PS4.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

This is fine for PS4, I'm just curious whether anyone knows if there's a 
predicate that means something like "target does not use GNU tools" so we don't 
have to itemize Fuschia and PS4 this way.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59815/new/

https://reviews.llvm.org/D59815



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

[PATCH] D59008: [AMDGPU] Switch default dwarf version to 5

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D59008#1442515 , @dblaikie wrote:

> In D59008#1442256 , @t-tye wrote:
>
> > In D59008#1442014 , @dblaikie 
> > wrote:
> >
> > > In D59008#1441903 , @t-tye wrote:
> > >
> > > > LGTM
> > > >
> > > > Do we know the state of split DWARF and DWARF compression for DWARF 5 
> > > > (compared to DWARF 2)?
> > >
> > >
> > > State of them in what sense? Compression is pretty orthogonal to any 
> > > DWARF version - it's more about the container (ELF, etc) you use. Split 
> > > DWARF is non-standardly supported in pre-v5, and I think it's functioning 
> > > in the standards conformant v5 mode too.
> >
> >
> > I had heard mention that at least split DWARF may not be fully supported 
> > yet for DWARF 5 in LLVM. But maybe that is old news:-)
>
>
> Might be - nothing I know of right now. (type units aren't fully supported in 
> DWARFv5 - but that's the only big thing on my list)


Anything having to do with section format and placement ought to be correct at 
this point.  Certainly I see v5 type units coming out in the .debug_info 
section per spec, and the .dwo files look right.  If there's something missing 
please file a bug and CC me.

There was certainly a release where split-dwarf and type units didn't work yet, 
but that's all done now.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59008/new/

https://reviews.llvm.org/D59008



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

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

As a rule I would prefer flags with positive names, as it's slightly easier to 
read `!isTrivial` than `!isNonTrivial`. And trivially shorter. :-)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59347/new/

https://reviews.llvm.org/D59347



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

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D59347#1444819 , @dblaikie wrote:

> In D59347#1443051 , @dblaikie wrote:
>
> > @asmith: Where's the LLVM-side change/review that goes with this, btw?
> >
> > In D59347#1442970 , @probinson 
> > wrote:
> >
> > > As a rule I would prefer flags with positive names, as it's slightly 
> > > easier to read `!isTrivial` than `!isNonTrivial`. And trivially shorter. 
> > > :-)
> >
> >
> > Fair enough - I was mostly coming at this from the "the patch that was 
> > committed should be reverted" & then we could haggle over other things, but 
> > fair point.
>
>
> Hmm, one other thought: Technically "non trivial" is perhaps more 
> accurate/less error prone. Only marking structures as "trivial" but other 
> types without that marker makes it more subtle (since not all trivial types 
> would be marked trivial - only those of a classification that means they 
> /could/ be non-trivial). Whereas marking the non-trivial types is more 
> broadly accurate.


Well, that's a reasonable point in favor of keeping the NonTrivial flag.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59347/new/

https://reviews.llvm.org/D59347



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

[PATCH] D59923: [Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Commentary questions.




Comment at: lib/Driver/ToolChains/Clang.cpp:3166
+  // If you say "-gsplit-dwarf -gline-tables-only", -gsplit-dwarf loses.
+  // But -gsplit-dwarf is not a g_Group option, hence we have to check the
+  // order explicitly. If -gsplit-dwarf wins, we fix DebugInfoKind later.

Regarding `But -gsplit-dwarf is not a g_Group option, hence we have to check 
the order explicitly.` It looks like when you added OPT_gsplit_dwarf to the 
if-statement above, we no longer have to check it explicitly?



Comment at: lib/Driver/ToolChains/Clang.cpp:3167
+  // But -gsplit-dwarf is not a g_Group option, hence we have to check the
+  // order explicitly. If -gsplit-dwarf wins, we fix DebugInfoKind later.
+  // This gets a bit more complicated if you've disabled inline info in

Regarding `If -gsplit-dwarf wins, we fix DebugInfoKind later.`
Is that still true?  It looks like -gsplit-dwarf is handled here now, because 
you removed the compensating code from lines 3268-3269.



Comment at: lib/Driver/ToolChains/Clang.cpp:3253
   // -gsplit-dwarf should turn on -g and enable the backend dwarf
   // splitting and extraction.
   if (T.isOSBinFormatELF()) {

This block of code no longer turns on -g?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59923/new/

https://reviews.llvm.org/D59923



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

[PATCH] D59923: [Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

[Paul Robinson via Phabricator via cfe-commits]

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

LGTM.  I wish it had occurred to me to pass both OPT_g_Group and 
OPT_gsplit_dwarf to the same getLastArgs call in the first place.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59923/new/

https://reviews.llvm.org/D59923



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

[PATCH] D59815: [Driver] Enable -fsanitize-address-globals-dead-stripping by default on PS4.

[Paul Robinson via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59815/new/

https://reviews.llvm.org/D59815



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

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: test/CodeGenCXX/debug-info-composite-triviality.cpp:88
-
-// CHECK-DAG: !DICompositeType({{.*}}, name: "NonTrivialE",{{.*}}flags: 
{{.*}}DIFlagNonTrivial
-struct NonTrivialE : Trivial, NonTrivial {

Hui wrote:
> probinson wrote:
> > This one used to be flagged nontrivial, and now it's not flagged?
> The base struct 'Trivial' was removed  and this case became duplicated since 
> it is now similar with 'struct NonTrivialD'. Could be very easy to add it 
> back per request.
I think it's worth having a multiple-inheritance case.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59347/new/

https://reviews.llvm.org/D59347



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

[PATCH] D58033: Add option for emitting dbg info for call sites

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I guess I'm not clear what your final goal is for the option.  Keep it, even 
though GCC doesn't have one like it?  Eliminate it?  Please clearly state what 
you intend to have in the end, and what you might have in the short term in 
case that is different.




Comment at: include/clang/Driver/CC1Options.td:362
 HelpText<"Use public LTO visibility for classes in std and stdext 
namespaces">;
+def emit_param_entry_values_cc1:
+Flag<["-"], "emit-param-entry-values-cc1">,

I think this is not necessary, see comment on Options.td.



Comment at: include/clang/Driver/Options.td:919
   HelpText<"Do not use jump tables for lowering switches">;
+def emit_param_entry_values : Joined<["-"], "femit-param-entry-values">,
+   Group,

djtodoro wrote:
> aprantl wrote:
> > I assume that this is the same -f option that GCC uses?
> Actually in GCC production of call_site and call_site_parameters debug info 
> is enabled by default.
> Production of entry_values is implemented on top of variable's value tracking 
> system and there is no particular option just for entry_values.
> 
> Basically, we introduce this option as experimental one. As soon as we test 
> everything we should get rid of this or turn it ON by default (and maybe add 
> '-fno-emit-param-entry-values' in order to have an option for disabling the 
> functionality). That will be ideal scenario.
By convention, the name of the option should start with 'f' and match the 
option spelling (with hyphens changed to underscore).



Comment at: include/clang/Driver/Options.td:921
+   Group,
+   Flags<[CC1Option]>,
+   HelpText<"Enables debug info about call site and call 
site parameters">;

I believe that by marking this with `[CC1Option]` cc1 will automatically 
understand it and you won't need to define a separate option in CC1Options.td.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58033/new/

https://reviews.llvm.org/D58033



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

[PATCH] D58033: Add option for emitting dbg info for call site parameters

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D58033#1470260 , @djtodoro wrote:

> @probinson @aprantl Thanks a lot for your comments!
>
> Let's clarify some things. I'm sorry about the confusion.
>
> Initial patch for the functionality can be restricted by this option (like we 
> pushed here), since **LLDB** doesn't read this type of debug info (yet).
>  The plan is, when  **LLDB** has all necessary info and we are sure that 
> everything works fine during using this info in debugging sessions, we can 
> turn it to `ON` by default.
>
> Does that make sense ? :)


Yes.  And I agree with Adrian, this should be a CC1-only option; it can be 
default-off for now and maybe debugger-tuning specific later.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58033/new/

https://reviews.llvm.org/D58033



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

[PATCH] D58033: Add option for emitting dbg info for call site parameters

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:3402
+CmdArgs.push_back("-femit-param-entry-values");
+
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC);

If this is now a cc1-only option, this part goes away right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58033/new/

https://reviews.llvm.org/D58033



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

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I'm okay with the PS4-specific bits.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60274/new/

https://reviews.llvm.org/D60274



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

[PATCH] D58033: Add option for emitting dbg info for call site parameters

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I'm okay with this, but give @aprantl a chance to confirm.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58033/new/

https://reviews.llvm.org/D58033



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

[PATCH] D60283: [DebugInfo] Don't emit checksums when compiling a preprocessed CPP

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I had tried to do this in r11 but some bots complained, so I reverted it 
and then didn't follow through.  Thanks for doing this!
When I tried it, I took advantage of existing tracking of line directives at 
the file level, so there shouldn't be a need to add a Line flag to PresumedLoc?
What I did was something like this, in computeChecksum():

  const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
  if (Invalid || !Entry.isFile() || Entry.getFile().hasLineDirectives())
return None;


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



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

[PATCH] D60283: [DebugInfo] Don't emit checksums when compiling a preprocessed CPP

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D60283#1480546 , @aganea wrote:

> Thanks Paul, your solution is even better. I'll apply rL11 
>  locally - if everything's fine, do you 
> mind if I re-land it again?


I suggest you do *not* use my patch unmodified, because it stops generating any 
checksums as soon as it finds one preprocessed file.  The functionality in your 
patch seems better, to skip checksums only for those files that look like they 
were preprocessed.   If you want to take my patch and remove the extra flag 
from CGDebugInfo that "remembers" that we've previously seen a file with line 
directives, that would be fine.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60283/new/

https://reviews.llvm.org/D60283



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

[PATCH] D61187: [clangd] Move clangd tests to clangd directory. check-clangd is no longer part of check-clang-tools.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

FYI, we had to disable clangd entirely after this patch, getting a weird 
problem with lit that we haven't figured out yet.
In case anybody else has seen something like this, and knows what's going on, 
please let us know.

Running all regression tests
llvm-lit.py: 
C:/j/w/opensource/opensource_to_sce_merge__2/o/llvm\utils\lit\lit\llvm\config.py:336:
 fatal: couldn't find 'clang' program, try setting CLANG in your environment
  C:\Program Files 
(x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(171,5): error 
MSB6006: "cmd.exe" exited with code 2. 
[C:\j\w\opensource\opensource_to_sce_merge__2\build\check-all.vcxproj]


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61187/new/

https://reviews.llvm.org/D61187



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

[PATCH] D61496: Fixed tests where grep was not matching the linefeed

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Checked-in files should not have CRLF endings, in general (there are a very few 
exceptions, and these aren't among them).
If the checked-in files have LF endings but your tool checks them out with CRLF 
then that is a problem with your config, or with the tool.
Adding dos2unix to the RUN lines is the wrong fix, either way.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61496/new/

https://reviews.llvm.org/D61496



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Do we generate the .dwo file directly these days?  If not, I can imagine 
wanting to avoid the overhead of the objcopy hack; as long as the linker is 
smart enough not to bother with the .debug_*.dwo sections this seems like a 
build-time win.


https://reviews.llvm.org/D52296



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

[PATCH] D57162: [DEBUG_INFO][NVPTX] Generate correct data about variable address class.

[Paul Robinson via Phabricator via cfe-commits]

probinson accepted this revision.
probinson added a comment.
This revision is now accepted and ready to land.
Herald added a project: clang.

LGTM. I'll trust you on the actual address-class values.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57162/new/

https://reviews.llvm.org/D57162



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

[PATCH] D57896: Variable names rule

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.
Herald added a project: LLVM.

In D57896#1389067 , @lebedev.ri wrote:

> 2. It might be best to give this more visibility, by submitting a mail to 
> llvm-dev, with a **noticeable** subject, like "RFC: changing variable naming 
> rules in LLVM codebase"


+1. I know the discussion took place there originally, but "RFC" will catch 
more people's eyes. Also the prior discussion had some digressions (ahem) which 
a new RFC can be more focused.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57896/new/

https://reviews.llvm.org/D57896



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

[PATCH] D58061: Fix a few tests that were missing ':' on CHECK lines and weren't testing anything.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

There has been some progress recently on better FileCheck diagnosis of likely 
test-writing issues, although to date it mostly requires the human to ask "what 
is going on here?" explicitly.  I can see adding a check to detect the 
missing-colon-on-otherwise-valid-directive case (including the usual suffixes), 
and my guess is that isn't too likely to cause excessive churn.  The deeper we 
go into typo-detection the more likely we are to trip over false positives, but 
this small step seems like a worthy change.
Happy to review such a patch, or somebody can file a PR and CC me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58061/new/

https://reviews.llvm.org/D58061



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

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: include/clang/AST/Decl.h:1482
 
+  bool getIsArgumentModified() const {
+return IsArgumentModified;

There should be a comment here.
The style in this class appears to omit the 'get' word from the name of the 
getter, so `isArgumentModified`  for the method name would look more consistent.



Comment at: lib/Sema/SemaExpr.cpp:11282
+static void EmitArgumentsValueModification(Expr *E) {
+  if (const DeclRefExpr *LHSDeclRef =
+  dyn_cast(E->IgnoreParenCasts()))

Does this fit on one line if you use `const auto *LHSDeclRef ...`? Given you 
have the dyn_cast on the RHS there's no real need to give the type name twice.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58035/new/

https://reviews.llvm.org/D58035



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

[PATCH] D57896: Variable names rule

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D57896#1406353 , @MyDeveloperDay 
wrote:

> I can't argue with anything you say.. but I guess to reinforce your point 
> introducing what is effectively a 3rd style would likely cause even more 
> jarring...


Zach isn't introducing a new style, the style already exists and is 
consistently used by what I think is our 3rd largest subproject.  It happens 
not to be used at all by the two largest subprojects, but those subprojects 
already aren't consistent with themselves.
I would not mind a more concerted effort to migrate to whatever style we pick, 
which was notably lacking last time around. Then the jarring inconsistencies 
would go away, and we could all get back to complaining about content and not 
style.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57896/new/

https://reviews.llvm.org/D57896



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

[PATCH] D57896: Variable names rule

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D57896#1406412 , @MyDeveloperDay 
wrote:

> In D57896#1406407 , @zturner wrote:
>
> > If I read the post correctly, it was actually agreeing with me (because it 
> > said "to reinforce your point...".  Meaning that something such as 
> > `lowerCaseCamel` would be the third style being referred to
>
>
> Correct! just acknowledging your point from a different perspective.


Doh!  Sorry for the noise.

It looks like the RFC thread has mostly turned into a transition-plan debate, 
so should we work on the actual convention description here? Extracting the 
naming conventions from the 3.6-era link mentioned above, we have:

- types and classes are UpperCamelCase (this is unchanged from current LLVM 
style)
- methods are UpperCamelCase (this is also the old LLVM style IIRC)
- variables are snake_case
- static data members add "g_" prefix to variable style (although I see a 
proposal for "s_" instead)
- nonstatic data members add "m_" prefix to variable style

Did I miss anything really important?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57896/new/

https://reviews.llvm.org/D57896



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

[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I would prefer a diagnostic if PS4 && >3.2.  Whether you map "latest" to 3.2 or 
diagnose that also, up to you, I'm okay either way.


Repository:
  rL LLVM

https://reviews.llvm.org/D36501



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

[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:2934
+ABICompatArg->render(Args, CmdArgs);
+
   // Add runtime flag for PS4 when PGO or Coverage are enabled.

```
else if (getToolChain().getTriple().isPS4())
  CmdArgs.push_back("-fclang-abi-compat=3.2");
```

Which lets us avoid piles of PS4 special cases all over everywhere.
Sony is historically very conservative about compatibility, and we'll be 
happier defaulting it this way.  Setting platform-specific defaults in the 
driver seems to be pretty common already, this is just one more.


Repository:
  rL LLVM

https://reviews.llvm.org/D36501



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

[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:2934
+ABICompatArg->render(Args, CmdArgs);
+
   // Add runtime flag for PS4 when PGO or Coverage are enabled.

rsmith wrote:
> probinson wrote:
> > ```
> > else if (getToolChain().getTriple().isPS4())
> >   CmdArgs.push_back("-fclang-abi-compat=3.2");
> > ```
> > 
> > Which lets us avoid piles of PS4 special cases all over everywhere.
> > Sony is historically very conservative about compatibility, and we'll be 
> > happier defaulting it this way.  Setting platform-specific defaults in the 
> > driver seems to be pretty common already, this is just one more.
> I initially thought that made sense too, but I'm now fairly convinced that it 
> doesn't.
> 
> 
> Let's take Darwin as an example. There are some facets of Darwin's platform 
> ABI that are determined by what old versions of Clang did, and other facets 
> of its platform ABI that have changed to match changes to the x86_64 psABI 
> and Itanium C++ ABI. But it's irrelevant where the platform ABI rules come 
> from; the point is that there is some set of platform ABI rules that you 
> could write down, and Clang attempts to implement those rules correctly by 
> default.
> 
> The flag being added in this patch provides the ability to request that Clang 
> does something else: that it produces code that is ABI-compatible with what a 
> prior version of itself did when targeting that platform ABI. In particular, 
> we fixed the C++ calling convention for certain rare class types in Clang 5 
> to conform to (an update to) the Itanium C++ ABI rules, and this patch allows 
> that to be undone.
> 
> It seems to me that the situation for PS4 is exactly the same. There is some 
> platform ABI that you could write down, and Clang attempts to be compatible 
> with that by default. And it's irrelevant whether that's the ABI that Clang 
> 3.2 used or the ABI of GCC 2.95; it's just the platform ABI. This is not a 
> "be compatible with clang 3.2" mode, it is (as far as I can tell) the actual 
> platform ABI.
> 
> 
> Let me repeat an example I gave before: suppose Clang 5 has some (accidental) 
> ABI change in it for PS4, and we fix that in Clang 6 to once again follow the 
> platform ABI by doing what Clang 3.2 did. In that case, building with 
> `-fclang-abi-compat=5` should theoretically reinstate that accidental ABI 
> change.
> 
> Hopefully that should clarify that this does *not* actually let us avoid PS4 
> special cases anywhere. ABI choices depend on both the platform ABI *and* on 
> the version of Clang that we're providing compatibility with (if any).
> 
> 
> That said, it's clearly not up to me what the PS4 platform ABI is. If you 
> want to say that the PS4 platform ABI is actually something other than what 
> Clang 3.2 does, but all object code on your system is compiled in a 
> compatibility mode that diverges from the platform ABI and matches Clang 3.2, 
> then I would concede that we can remove the PS4 platform special cases 
> elsewhere and set a default here. But that sounds like a very strange 
> decision to make, and it creates a horrible problem for the meaning of the 
> `-fclang-abi-compat` flag: if someone in the future specifies 
> `-fclang-abi-compat=5` when targeting PS4, and Clang 5 by default set 
> `-fclang-abi-compat=3.2`, then are we targeting what Clang 5 would have done 
> by default or what Clang 5 would have done when told to be compatible with 
> itself? As you can see, this default would create a lot of confusion.
On the other hand, if all the places that check ClangABICompat also check for 
PS4 and Darwin, then specifying `-fclang-abi-compat` while targeting PS4 or 
Darwin has no effect, and also no diagnostic.  Which seems to make 
`-fclang-abi-compat` totally pointless.  Is there a non-PS4/Darwin use-case for 
this flag?


Repository:
  rL LLVM

https://reviews.llvm.org/D36501



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

[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: lib/CodeGen/CGVTables.cpp:157
+  if (DebugInfo)
+DebugInfo->replaceTemporaryNodes();
+

aprantl wrote:
> Have you looked at what it would take to only finalize the nodes reachable 
> via the function?
> Otherwise — have you audited that this is always safe?
I do not know how to find nodes reachable from a particular function, either 
before or after they are finalized.

GenerateVarArgsThunk is called after we do EmitGlobalFunctionDefinition on the 
function we are about to clone.  The ReplaceMap holds replaceable forward type 
declarations, I think?  So I can imagine that codegen for a subsequent function 
might need to create a complete type, even one that is reachable from the 
function we are about to clone.

Of course I find the whole metadata memory management scheme pretty opaque, but 
this is my best guess about the situation.


https://reviews.llvm.org/D37038



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

[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Sorry it took so long to wrap my head around this, but it has been about 20 
years since I've had to accommodate an intentional ABI breakage, and that's 
actually what you want to do.  Remembering that case, I have a model for this 
one, and I understand what you are doing now.

Be that as it may, `-fclang-abi-compat` is meaningless on PS4, because every 
time there's a case for checking ClangABICompat, PS4 will want to do it the 3.2 
way regardless.  And we'll just make those checks in all those places.  At 
least it will be relatively easy to audit, and I'm sure our licensees will be 
happy to ignore the new flag.

So, I am convinced, and the patch LGTM.


Repository:
  rL LLVM

https://reviews.llvm.org/D36501



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

[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Trying to understand the broader context here, I looked back through the list 
of revisions mentioned in PR33930 to see if that helped.

When called on a method, CodeGenModule::EmitGlobalDefinition() calls 
CodeGenModule::EmitGlobalFunctionDefinition(), which in turn calls 
CodeGenFunction::GenerateCode(), which calls CodeGenFunction::FinishFunction(), 
which calls CGDebugInfo::EmitFunctionEnd(), which calls 
DIBuilder::finalizeSubprogram().  This is supposed to finalize the metadata for 
the subprogram.
If the method is virtual, EmitGlobalDefinition() then calls 
getVTables().EmitThunks() which eventually gets to GenerateVarArgsThunk().  
Which crashes when it tries to CloneFunction the original virtual method, 
because an operand of some piece of metadata is still temporary.

So, either something happens such that EmitFunctionEnd() doesn't actually call 
finalizeSubprogram(), or finalizeSubprogram() doesn't finalize everything that 
it needs to finalize.

finalizeSubprogram() retrieves the variables from the subprogram and handles 
them; what is it missing?


https://reviews.llvm.org/D37038



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

[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In https://reviews.llvm.org/D37038#854722, @probinson wrote:

> finalizeSubprogram() retrieves the variables from the subprogram and handles 
> them; what is it missing?


For temp-md-nodes2.cpp, the assertion in mapTopLevelUniquedNode() trips on a 
DICompositeType for CBdVfsImpl.  This appears to be the "scope" operand of the 
(distinct) DISubprogram for "ReqCacheHint" which is the function being cloned.

For temp-md-nodes1.cpp, the assertion in visitOperands() trips on a 
DICompositeType for "Charlie" but I haven't worked out where it's used yet.  Is 
there a straightforward way to dump all the metadata for a function?  Calling 
`print()` or 'dump()` just does the one node.


https://reviews.llvm.org/D37038



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

[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Dumping the whole module, the temporary DICompositeType node for CBdVfsImpl is 
used both as the scope node for the DISubprogram, and also as the baseType of a 
DIDerivedType which is a pointer type in the type list for the subprogram.

Given that the DICompositeType is a scope for the method, but is still marked 
as a forward declaration, I'm guessing that the composite type will be expanded 
into a full type, but that apparently happens after codegen for its methods.  
So, it would be premature to finalize the metadata node at this point?


https://reviews.llvm.org/D37038



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