[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: docs/CodingStandards.rst:514-516
+line of code. Some common editors will automatically remove trailing whitespace
+when saving a file which causes unrelated changes to appear in diffs and
+commits.

chandlerc wrote:
> Meinersbur wrote:
> > Just to note that this policy will prohibit the use of remove 
> > trailing-whitespace feature in editors since it makes it impossible to edit 
> > the file without also 'editing' any unrelated whitespace. At the same time, 
> > since not being able to use the feature, I risk committing trailing 
> > whitespace myself (that is, some additional steps are necessary: I use 'git 
> > clang-format' which should remove traling whitespace only on edited lines 
> > and 'git diff' shows trailing whitespace in red; these are still additional 
> > steps that are easy to miss)
> I also have editor settings that risk violating this, but I just reduce my 
> patdh before submitting. This is a touch annoying with svn, but with got it 
> is pretty simple to use `git add -p` and ignore the unnecessary removals if 
> trailing whitespace 
I had the same workflow as Chandler but that became rather tedious for the LLDB 
repo where there's a lot of trailing whitespace. I ended up adding an alias to 
my git config that only stages non-whitespace changes: `anw = !sh -c 'git diff 
-U0 -w --no-color "$@" | git apply --cached --ignore-whitespace --unidiff-zero 
-'`. It's far from optimal but it works pretty well. 


https://reviews.llvm.org/D50055



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

[PATCH] D49549: Change 'clang-test' to 'check-clang' on the hacking webpage

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Looks good, thanks!

PS: I don't think it hurts, but it's sufficient to add cfe-commits as a 
subscriber. I believe it's an automation account (so it won't be able to accept 
on phab) and replies via e-mail are posted under the corresponding user's 
account.


Repository:
  rC Clang

https://reviews.llvm.org/D49549



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

[PATCH] D46694: [diagtool] Install diagtool

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: arphaman, dexonsmith, jkorous, rsmith.
Herald added a subscriber: mgorny.

Although not very well known, diagtool is an incredibly convenient utility for 
dealing with diagnostics. I believe it's worth adding this to the install 
target.


Repository:
  rC Clang

https://reviews.llvm.org/D46694

Files:
  clang/tools/diagtool/CMakeLists.txt


Index: clang/tools/diagtool/CMakeLists.txt
===
--- clang/tools/diagtool/CMakeLists.txt
+++ clang/tools/diagtool/CMakeLists.txt
@@ -17,3 +17,15 @@
   clangBasic
   clangFrontend
   )
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  install(TARGETS diagtool
+COMPONENT diagtool
+RUNTIME DESTINATION bin)
+
+  if (NOT CMAKE_CONFIGURATION_TYPES)
+add_llvm_install_targets(install-diagtool
+  DEPENDS diagtool
+  COMPONENT diagtool)
+  endif()
+endif()


Index: clang/tools/diagtool/CMakeLists.txt
===
--- clang/tools/diagtool/CMakeLists.txt
+++ clang/tools/diagtool/CMakeLists.txt
@@ -17,3 +17,15 @@
   clangBasic
   clangFrontend
   )
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  install(TARGETS diagtool
+COMPONENT diagtool
+RUNTIME DESTINATION bin)
+
+  if (NOT CMAKE_CONFIGURATION_TYPES)
+add_llvm_install_targets(install-diagtool
+  DEPENDS diagtool
+  COMPONENT diagtool)
+  endif()
+endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46694: [diagtool] Install diagtool

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D46694#1094617, @dexonsmith wrote:

> This SGTM, but I wouldn't mind hearing from others.  I wonder if this is 
> worth a quick RFC on cfe-dev?


Sounds good, I've created a thread here: 
http://lists.llvm.org/pipermail/cfe-dev/2018-May/057946.html


Repository:
  rC Clang

https://reviews.llvm.org/D46694



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

[PATCH] D46694: [diagtool] Install diagtool

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL332448: [diagtool] Add diagtool to install target. (authored 
by JDevlieghere, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46694?vs=146129&id=147036#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D46694

Files:
  cfe/trunk/docs/CommandGuide/diagtool.rst
  cfe/trunk/docs/CommandGuide/index.rst
  cfe/trunk/docs/ReleaseNotes.rst
  cfe/trunk/tools/diagtool/CMakeLists.txt

Index: cfe/trunk/tools/diagtool/CMakeLists.txt
===
--- cfe/trunk/tools/diagtool/CMakeLists.txt
+++ cfe/trunk/tools/diagtool/CMakeLists.txt
@@ -17,3 +17,15 @@
   clangBasic
   clangFrontend
   )
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  install(TARGETS diagtool
+COMPONENT diagtool
+RUNTIME DESTINATION bin)
+
+  if (NOT CMAKE_CONFIGURATION_TYPES)
+add_llvm_install_targets(install-diagtool
+  DEPENDS diagtool
+  COMPONENT diagtool)
+  endif()
+endif()
Index: cfe/trunk/docs/CommandGuide/index.rst
===
--- cfe/trunk/docs/CommandGuide/index.rst
+++ cfe/trunk/docs/CommandGuide/index.rst
@@ -15,3 +15,4 @@
:maxdepth: 1
 
clang
+   diagtool
Index: cfe/trunk/docs/CommandGuide/diagtool.rst
===
--- cfe/trunk/docs/CommandGuide/diagtool.rst
+++ cfe/trunk/docs/CommandGuide/diagtool.rst
@@ -0,0 +1,52 @@
+diagtool - clang diagnostics tool
+=
+
+SYNOPSIS
+
+
+:program:`diagtool` *command* [*args*]
+
+DESCRIPTION
+---
+
+:program:`diagtool` is a combination of four tool for dealing with diagnostics in :program:`clang`.
+
+SUBCOMMANDS
+---
+
+:program:`diagtool` is separated into several subcommands each tailored to a
+different purpose. A brief summary of each command follows, with more detail in
+the sections that follow.
+
+  * :ref:`find_diagnostic_id` - Print the id of the given diagnostic.
+  * :ref:`list_warnings` - List warnings and their corresponding flags.
+  * :ref:`show_enabled` - Show which warnings are enabled for a given command line.
+  * :ref:`tree` - Show warning flags in a tree view.
+
+.. _find_diagnostic_id:
+
+find-diagnostic-id
+~~
+
+:program:`diagtool` find-diagnostic-id *diagnostic-name*
+
+.. _list_warnings:
+
+list-warnings
+~
+
+:program:`diagtool` list-warnings
+
+.. _show_enabled:
+
+show-enabled
+
+
+:program:`diagtool` show-enabled [*options*] *filename ...*
+
+.. _tree:
+
+tree
+
+
+:program:`diagtool` tree [*diagnostic-group*]
Index: cfe/trunk/docs/ReleaseNotes.rst
===
--- cfe/trunk/docs/ReleaseNotes.rst
+++ cfe/trunk/docs/ReleaseNotes.rst
@@ -93,6 +93,11 @@
   behavior can be restored by setting ``-fclang-abi-compat`` to ``6`` or
   lower.
 
+- An existing tool named ``diagtool`` has been added to the release. As the
+  name suggests, it helps with dealing with diagnostics in ``clang``, such as
+  finding out the warning hierarchy, and which of them are enabled by default
+  or for a particular compiler invocation.
+
 - ...
 
 New Compiler Flags
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Thanks David, LGTM


https://reviews.llvm.org/D45686



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

[PATCH] D47190: [Driver] Add -ivfsoverlay-lib option to load VFS from shared library

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I don't know much about the VFS so I only did a superficial review.




Comment at: include/clang/Lex/HeaderSearchOptions.h:176
 
+  /// \brief The set of libraries with user-provided virtual filesystem.
+  std::vector VFSOverlayLibs;

We now have autobrief enabled so you can omit the `\brief`. 



Comment at: lib/Frontend/CompilerInvocation.cpp:3218
+  for (const std::string &LibFile : CI.getHeaderSearchOpts().VFSOverlayLibs) {
+std::string Error;
+auto Lib = llvm::sys::DynamicLibrary::getPermanentLibrary(

Would it be useful to print the contents of the error?



Comment at: unittests/Frontend/VfsTestLib.cpp:24
+  std::error_code EC = llvm::sys::fs::make_absolute(FileName);
+  if (EC) {
+return nullptr;

The llvm coding standard says no braces for single line statements. 


Repository:
  rC Clang

https://reviews.llvm.org/D47190



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I very much like this check. I only have a few minor comments, but maybe this 
encourages others to have a look too!




Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:85
+diag(ElseIfWithoutElse->getLocStart(),
+ "potential uncovered codepath found; add an ending else branch");
+return;

I'm not a big fan of the 'found', can we just omit it? The same goes for the 
other diags. 



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:96
+// Only the default branch (we explicitly matched for default!) exists.
+if (CaseCount == 1) {
+  diag(SwitchWithDefault->getLocStart(),

Why not a switch?



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:152
+// Should be written as an IfStmt.
+if (CaseCount == 1) {
+  diag(SwitchWithoutDefault->getLocStart(), "switch stmt with only one "

I'm aware that the message and fixme are different, but since the structure is 
so similar to the handling of the other switch case, I wonder if there is any 
chance we could extract the common parts?


https://reviews.llvm.org/D37808



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:96
+// Only the default branch (we explicitly matched for default!) exists.
+if (CaseCount == 1) {
+  diag(SwitchWithDefault->getLocStart(),

JonasToth wrote:
> JDevlieghere wrote:
> > Why not a switch?
> I intent to check if all cases are explicitly covered.
> 
> In the testcases there is one switch with all numbers explicitly written, 
> meaning there is no need to add a default branch.
> 
> This would allow further 
> ```
> else if (CaseCount == MaximumPossibleCases) { /* No warning */ }
> ```
> path which is not modable with `switch`.
Sounds good



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:152
+// Should be written as an IfStmt.
+if (CaseCount == 1) {
+  diag(SwitchWithoutDefault->getLocStart(), "switch stmt with only one "

JonasToth wrote:
> JDevlieghere wrote:
> > I'm aware that the message and fixme are different, but since the structure 
> > is so similar to the handling of the other switch case, I wonder if there 
> > is any chance we could extract the common parts?
> I try to get something shorter.
> Maybe 
> ```
> if(CaseCount == 1 && MatchedSwitch) {}
> else if(CaseCount == 1 && MatchedElseIf) {}
> ```
> ?
Wouldn't it be easier to have a function and pass as arguments the stuff that's 
different? Both are `SwitchStmt`s so if you pass that + the diagnostic message 
you should be able to share the other logic. 


https://reviews.llvm.org/D37808



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:76
+
+std::size_t twoPow(std::size_t Bits) {
+  const std::size_t DiscreteValues = 1ul << Bits;

Add a comment describing what this function does. I'd move and rephrase the 
comment below. 



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:98
+  // hold.
+  const std::size_t BitCount = [&T, &Context]() {
+if (T->isIntegralType(Context))

Unless you expect a whole bunch of logic to be added here, I'd un-const and 
initialize BitCount to zero, then just have if-clause reassign it and get rid 
of the lambda. This will save you a few lines of code and complexity. 



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:149
+
+  llvm::SmallVector DegenerateMsgs = {
+  "degenerated switch with default label only",

If there's only going to be two messages, you could use the ternary operator 
and save an instantiation of the `SmallVector`.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:195
+  // matcher used for here does not match on degenerate 'switch'
+  assert(CaseCount > 0 && "Switch stmt without any case found. This case "
+  "should be excluded by the matcher and is handled "

Let's move this to right after where you define CaseCount 


https://reviews.llvm.org/D37808



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

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:80
+
+  // Unsigned overflow occured. Returning max() is sufficient, since noone
+  // writes so many case labels in source code.

Maybe merge this with the function comment, no point in having both imho.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:82
+  // writes so many case labels in source code.
+  if (Bits > 0 && DiscreteValues <= 1) {
+return std::numeric_limits::max();

LLVM style guide says no braces with single line if/else statements. No need 
for the else after a return. Alternatively you could use the ternary operator. 



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:100
+return twoPow(Context.getTypeSize(T));
+  else
+return twoPow(0ul);

No need for the else here. See previous comment. 



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:146
+
+  if (CaseCount == 1 || CaseCount == 2) {
+diag(Switch->getLocStart(),

Braces



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:180
+if (const auto *IntegerCondition =
+Result.Nodes.getNodeAs("non-enum-condition")) {
+  return getNumberOfPossibleValues(IntegerCondition->getType(),

Braces



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:183
+   *Result.Context);
+} else if (const auto *BitfieldDecl =
+   Result.Nodes.getNodeAs("bitfield")) {

You can just use if because of the return.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:186
+  return twoPow(BitfieldDecl->getBitWidthValue(*Result.Context));
+} else {
+  llvm_unreachable("either bitfield or non-enum must be condition");

Drop the else



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:198
+  // missed some value explicity, therefore add a default branch.
+  if (CaseCount < MaxPathsPossible) {
+diag(Switch->getLocStart(), CoverMsgs[CaseCount == 1 ? 0 : 1]);

Braces


https://reviews.llvm.org/D37808



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

[PATCH] D51485: [Sema][NFC] Trivial cleanup in ActOnCallExpr

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Nice cleanup, LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D51485



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

[PATCH] D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Assuming `ArgExprs` doesn't change between those two calls this seems fine.




Comment at: Sema/SemaExpr.cpp:5338
 Context.DependentTy, VK_RValue, RParenLoc);
   } else {
 

While you're at it you might as well remove the else branch here.


Repository:
  rC Clang

https://reviews.llvm.org/D51488



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

[PATCH] D42370: Issue local statics in correct DWARF lexical scope

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision as: JDevlieghere.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

This makes sense to me, LGTM.


https://reviews.llvm.org/D42370



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

[PATCH] D51807: Remove all uses of DIFlagBlockByrefStruct from Clang

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision as: JDevlieghere.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

Thanks Adrian, LGTM!


https://reviews.llvm.org/D51807



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

[PATCH] D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: Sema/SemaExpr.cpp:5338
 Context.DependentTy, VK_RValue, RParenLoc);
   } else {
 

jkorous wrote:
> JDevlieghere wrote:
> > While you're at it you might as well remove the else branch here.
> Sorry, I am not following. Do you mean just refactoring?
> 
> ```
> if a
>   foo
> else
>   bar
> 
> ```
> ->
> ```
> if a
>   foo
> 
> bar
> ```
> 
> Or do you really mean removing the else branch? I don't see how that would be 
> NFC.
Yup, that's what I meant. since we have a return anyway. 


Repository:
  rC Clang

https://reviews.llvm.org/D51488



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

[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: aprantl, dexonsmith.

We generate incorrect values for the DW_AT_data_bit_offset for interfaces in 
Objective-C. I can only speculate as to what we were trying to achieve by 
taking the modulo of the bit size with the byte size, but given that the size 
and offset is expressed in bits, this seems wrong.


Repository:
  rC Clang

https://reviews.llvm.org/D51990

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenObjC/debug-info-ivars.m


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -15,30 +15,29 @@
 }
 @end
 
-@implementation BaseClass
-@end
+@implementation BaseClass
+@end
 
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
-// CHECK-SAME:   line: 10
-// CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32,
-// CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: ![[INT]] = !DIBasicType(name: "int"
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
-// CHECK-SAME:   line: 11
-// CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
-// CHECK-SAME:   size: 9,
-// CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
-// CHECK-SAME:   line: 12
-// CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, offset: 1,
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
-// CHECK-SAME:   line: 14
-// CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, offset: 3,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
+// CHECK-SAME:   line: 10
+// CHECK-SAME:   baseType: ![[INT:[0-9]+]]
+// CHECK-SAME:   size: 32,
+// CHECK-NOT:offset:
+// CHECK-SAME:   flags: DIFlagProtected
+// CHECK: ![[INT]] = !DIBasicType(name: "int"
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
+// CHECK-SAME:   line: 11
+// CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
+// CHECK-SAME:   size: 9, offset: 96
+// CHECK-SAME:   flags: DIFlagProtected
+// CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
+// CHECK-SAME:   line: 12
+// CHECK-SAME:   baseType: ![[UNSIGNED]]
+// CHECK-SAME:   size: 9, offset: 105
+// CHECK-SAME:   flags: DIFlagProtected
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
+// CHECK-SAME:   line: 14
+// CHECK-SAME:   baseType: ![[UNSIGNED]]
+// CHECK-SAME:   size: 9, offset: 115
+// CHECK-SAME:   flags: DIFlagProtected
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2363,13 +2363,10 @@
   // We don't know the runtime offset of an ivar if we're using the
   // non-fragile ABI.  For bitfields, use the bit offset into the first
   // byte of storage of the bitfield.  For other fields, use zero.
-  if (Field->isBitField()) {
-FieldOffset =
-CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field);
-FieldOffset %= CGM.getContext().getCharWidth();
-  } else {
-FieldOffset = 0;
-  }
+  FieldOffset =
+  Field->isBitField()
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {
   FieldOffset = RL.getFieldOffset(FieldNo);
 }


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -15,30 +15,29 @@
 }
 @end
 
-@implementation BaseClass
-@end
+@implementation BaseClass
+@end
 
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
-// CHECK-SAME:   line: 10
-// CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32,
-// CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: ![[INT]] = !DIBasicType(name: "int"
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
-// CHECK-SAME:   line: 11
-// CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
-// CHECK-SAME:   size: 9,
-// CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned 

[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {

aprantl wrote:
> aprantl wrote:
> > It might help to attempt some git blame archeology.
> > Judging from the comment, it sounds like the debugger is supposed to query 
> > the runtime for the *byte* offset and then add the bit offset from DWARF? 
> > Could that make sense?
> If that is the case, we'd need to relax llvm-dwarfdump --verify to accept 
> this and make sure LLDB does the right thing instead.
Ah I see, yeah that sounds reasonable and explains the comment which I 
interpreted differently. Thanks! 


Repository:
  rC Clang

https://reviews.llvm.org/D51990



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

[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {

JDevlieghere wrote:
> aprantl wrote:
> > aprantl wrote:
> > > It might help to attempt some git blame archeology.
> > > Judging from the comment, it sounds like the debugger is supposed to 
> > > query the runtime for the *byte* offset and then add the bit offset from 
> > > DWARF? Could that make sense?
> > If that is the case, we'd need to relax llvm-dwarfdump --verify to accept 
> > this and make sure LLDB does the right thing instead.
> Ah I see, yeah that sounds reasonable and explains the comment which I 
> interpreted differently. Thanks! 
btw it was added in rL167503. 


Repository:
  rC Clang

https://reviews.llvm.org/D51990



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

[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {

aprantl wrote:
> JDevlieghere wrote:
> > JDevlieghere wrote:
> > > aprantl wrote:
> > > > aprantl wrote:
> > > > > It might help to attempt some git blame archeology.
> > > > > Judging from the comment, it sounds like the debugger is supposed to 
> > > > > query the runtime for the *byte* offset and then add the bit offset 
> > > > > from DWARF? Could that make sense?
> > > > If that is the case, we'd need to relax llvm-dwarfdump --verify to 
> > > > accept this and make sure LLDB does the right thing instead.
> > > Ah I see, yeah that sounds reasonable and explains the comment which I 
> > > interpreted differently. Thanks! 
> > btw it was added in rL167503. 
> We should check whether emitting the offsets like this violates the DWARF 
> spec. If yes, it may be better to emit the static offsets like you are doing 
> here and then still have LLDB ignore everything but the bit-offsets from the 
> dynamic byte offset.
The standard says 

> The member entry corresponding to a data member that is defined in a 
> structure,
> union or class may have either a DW_AT_data_member_location attribute or a
> DW_AT_data_bit_offset attribute.

which to me sounds like they should be mutually exclusive. I ran the lldb test 
suite with my change and there were no new failures, which leads me to believe 
that the comment from r167503 still holds and lldb ignores this attribute, at 
least for Objective-C.


Repository:
  rC Clang

https://reviews.llvm.org/D51990



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

[PATCH] D52058: Add Parameters to DW_AT_name Attribute of Template Variables

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:3126
+  } else {
+templateParameters = nullptr;//llvm::DINodeArray().get();
+  }

What's the meaning of this comment? 



Comment at: lib/CodeGen/CGDebugInfo.h:654
+   StringRef &LinkageName,
+   llvm::MDTuple *&templateParameters,
+   llvm::DIScope *&VDContext);

s/templateParameters/TemplateParameters/ (same for the rest of this patch)


Repository:
  rC Clang

https://reviews.llvm.org/D52058



___
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.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: labath, dblaikie, probinson.

Currently, support for debug_types is only present for ELF and trying to pass 
-fdebug-types-section for other targets results in a crash in the backend. 
Until this is fixed, we should emit a diagnostic in the front end when the 
option is passed for non-linux targets.


Repository:
  rC Clang

https://reviews.llvm.org/D49594

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/debug-options.c


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -139,12 +139,18 @@
 //
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck 
-check-prefix=GARANGE %s
 //
-// RUN: %clang -### -fdebug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 
\
 // RUN:| FileCheck -check-prefix=FDTS %s
 //
-// RUN: %clang -### -fdebug-types-section -fno-debug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target 
x86_64-unknown-linux %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOFDTS %s
 //
+// RUN: %clang -### -fdebug-types-section -target x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=FDTSE %s
+//
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target 
x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NOFDTSE %s
+//
 // RUN: %clang -### -g -gno-column-info %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOCI %s
 //
@@ -229,8 +235,10 @@
 // GARANGE: -generate-arange-section
 //
 // FDTS: "-mllvm" "-generate-type-units"
+// FDTSE: error: unsupported option '-fdebug-types-section' for target 
'x86_64-apple-darwin'
 //
 // NOFDTS-NOT: "-mllvm" "-generate-type-units"
+// NOFDTSE-NOT: error: unsupported option '-fdebug-types-section' for target 
'x86_64-apple-darwin'
 //
 // CI: "-dwarf-column-info"
 //
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3028,6 +3028,11 @@
 
   if (Args.hasFlag(options::OPT_fdebug_types_section,
options::OPT_fno_debug_types_section, false)) {
+if (!T.isOSLinux())
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << Args.getLastArg(options::OPT_fdebug_types_section)
+ ->getAsString(Args)
+  << T.getTriple();
 CmdArgs.push_back("-mllvm");
 CmdArgs.push_back("-generate-type-units");
   }


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -139,12 +139,18 @@
 //
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=GARANGE %s
 //
-// RUN: %clang -### -fdebug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 \
 // RUN:| FileCheck -check-prefix=FDTS %s
 //
-// RUN: %clang -### -fdebug-types-section -fno-debug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target x86_64-unknown-linux %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOFDTS %s
 //
+// RUN: %clang -### -fdebug-types-section -target x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=FDTSE %s
+//
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NOFDTSE %s
+//
 // RUN: %clang -### -g -gno-column-info %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOCI %s
 //
@@ -229,8 +235,10 @@
 // GARANGE: -generate-arange-section
 //
 // FDTS: "-mllvm" "-generate-type-units"
+// FDTSE: error: unsupported option '-fdebug-types-section' for target 'x86_64-apple-darwin'
 //
 // NOFDTS-NOT: "-mllvm" "-generate-type-units"
+// NOFDTSE-NOT: error: unsupported option '-fdebug-types-section' for target 'x86_64-apple-darwin'
 //
 // CI: "-dwarf-column-info"
 //
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3028,6 +3028,11 @@
 
   if (Args.hasFlag(options::OPT_fdebug_types_section,
options::OPT_fno_debug_types_section, false)) {
+if (!T.isOSLinux())
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << Args.getLastArg(options::OPT_fdebug_types_section)
+ ->getAsString(Args)
+  << T.getTriple();
 CmdArgs.push_back("-mllvm");
 CmdArgs.push_back("-generate-type-units");
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Please see PR38190 for more details.


Repository:
  rC Clang

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.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 156471.
JDevlieghere added a comment.

Thanks, I meant to use `isOSBinFormatELF` but I think it got lost in an 
accidental undo. The test didn't capture that because it's using a linux target 
triple.


https://reviews.llvm.org/D49594

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/debug-options.c


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -139,12 +139,18 @@
 //
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck 
-check-prefix=GARANGE %s
 //
-// RUN: %clang -### -fdebug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 
\
 // RUN:| FileCheck -check-prefix=FDTS %s
 //
-// RUN: %clang -### -fdebug-types-section -fno-debug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target 
x86_64-unknown-linux %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOFDTS %s
 //
+// RUN: %clang -### -fdebug-types-section -target x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=FDTSE %s
+//
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target 
x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NOFDTSE %s
+//
 // RUN: %clang -### -g -gno-column-info %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOCI %s
 //
@@ -229,8 +235,10 @@
 // GARANGE: -generate-arange-section
 //
 // FDTS: "-mllvm" "-generate-type-units"
+// FDTSE: error: unsupported option '-fdebug-types-section' for target 
'x86_64-apple-darwin'
 //
 // NOFDTS-NOT: "-mllvm" "-generate-type-units"
+// NOFDTSE-NOT: error: unsupported option '-fdebug-types-section' for target 
'x86_64-apple-darwin'
 //
 // CI: "-dwarf-column-info"
 //
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3028,6 +3028,11 @@
 
   if (Args.hasFlag(options::OPT_fdebug_types_section,
options::OPT_fno_debug_types_section, false)) {
+if (!T.isOSBinFormatELF())
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << Args.getLastArg(options::OPT_fdebug_types_section)
+ ->getAsString(Args)
+  << T.getTriple();
 CmdArgs.push_back("-mllvm");
 CmdArgs.push_back("-generate-type-units");
   }


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -139,12 +139,18 @@
 //
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=GARANGE %s
 //
-// RUN: %clang -### -fdebug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 \
 // RUN:| FileCheck -check-prefix=FDTS %s
 //
-// RUN: %clang -### -fdebug-types-section -fno-debug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target x86_64-unknown-linux %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOFDTS %s
 //
+// RUN: %clang -### -fdebug-types-section -target x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=FDTSE %s
+//
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NOFDTSE %s
+//
 // RUN: %clang -### -g -gno-column-info %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOCI %s
 //
@@ -229,8 +235,10 @@
 // GARANGE: -generate-arange-section
 //
 // FDTS: "-mllvm" "-generate-type-units"
+// FDTSE: error: unsupported option '-fdebug-types-section' for target 'x86_64-apple-darwin'
 //
 // NOFDTS-NOT: "-mllvm" "-generate-type-units"
+// NOFDTSE-NOT: error: unsupported option '-fdebug-types-section' for target 'x86_64-apple-darwin'
 //
 // CI: "-dwarf-column-info"
 //
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3028,6 +3028,11 @@
 
   if (Args.hasFlag(options::OPT_fdebug_types_section,
options::OPT_fno_debug_types_section, false)) {
+if (!T.isOSBinFormatELF())
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << Args.getLastArg(options::OPT_fdebug_types_section)
+ ->getAsString(Args)
+  << T.getTriple();
 CmdArgs.push_back("-mllvm");
 CmdArgs.push_back("-generate-type-units");
   }
___
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.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D49594#1169809, @probinson wrote:

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


It's more that I want to reflect the current situation and prevent MC from 
crashing while we come up with a solution. This was very little work and we can 
always revert it once things are fixed.


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.

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337717: [DebugInfo] Error out when enabling 
-fdebug-types-section on non-ELF target. (authored by JDevlieghere, committed 
by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49594?vs=156471&id=156825#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49594

Files:
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/test/Driver/debug-options.c


Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3028,6 +3028,11 @@
 
   if (Args.hasFlag(options::OPT_fdebug_types_section,
options::OPT_fno_debug_types_section, false)) {
+if (!T.isOSBinFormatELF())
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << Args.getLastArg(options::OPT_fdebug_types_section)
+ ->getAsString(Args)
+  << T.getTriple();
 CmdArgs.push_back("-mllvm");
 CmdArgs.push_back("-generate-type-units");
   }
Index: cfe/trunk/test/Driver/debug-options.c
===
--- cfe/trunk/test/Driver/debug-options.c
+++ cfe/trunk/test/Driver/debug-options.c
@@ -139,12 +139,18 @@
 //
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck 
-check-prefix=GARANGE %s
 //
-// RUN: %clang -### -fdebug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 
\
 // RUN:| FileCheck -check-prefix=FDTS %s
 //
-// RUN: %clang -### -fdebug-types-section -fno-debug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target 
x86_64-unknown-linux %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOFDTS %s
 //
+// RUN: %clang -### -fdebug-types-section -target x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=FDTSE %s
+//
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target 
x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NOFDTSE %s
+//
 // RUN: %clang -### -g -gno-column-info %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOCI %s
 //
@@ -229,8 +235,10 @@
 // GARANGE: -generate-arange-section
 //
 // FDTS: "-mllvm" "-generate-type-units"
+// FDTSE: error: unsupported option '-fdebug-types-section' for target 
'x86_64-apple-darwin'
 //
 // NOFDTS-NOT: "-mllvm" "-generate-type-units"
+// NOFDTSE-NOT: error: unsupported option '-fdebug-types-section' for target 
'x86_64-apple-darwin'
 //
 // CI: "-dwarf-column-info"
 //


Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3028,6 +3028,11 @@
 
   if (Args.hasFlag(options::OPT_fdebug_types_section,
options::OPT_fno_debug_types_section, false)) {
+if (!T.isOSBinFormatELF())
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << Args.getLastArg(options::OPT_fdebug_types_section)
+ ->getAsString(Args)
+  << T.getTriple();
 CmdArgs.push_back("-mllvm");
 CmdArgs.push_back("-generate-type-units");
   }
Index: cfe/trunk/test/Driver/debug-options.c
===
--- cfe/trunk/test/Driver/debug-options.c
+++ cfe/trunk/test/Driver/debug-options.c
@@ -139,12 +139,18 @@
 //
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=GARANGE %s
 //
-// RUN: %clang -### -fdebug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 \
 // RUN:| FileCheck -check-prefix=FDTS %s
 //
-// RUN: %clang -### -fdebug-types-section -fno-debug-types-section %s 2>&1 \
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target x86_64-unknown-linux %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOFDTS %s
 //
+// RUN: %clang -### -fdebug-types-section -target x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=FDTSE %s
+//
+// RUN: %clang -### -fdebug-types-section -fno-debug-types-section -target x86_64-apple-darwin %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NOFDTSE %s
+//
 // RUN: %clang -### -g -gno-column-info %s 2>&1 \
 // RUN:| FileCheck -check-prefix=NOCI %s
 //
@@ -229,8 +235,10 @@
 // GARANGE: -generate-arange-section
 //
 // FDTS: "-mllvm" "-generate-type-units"
+// FDTSE: error: unsupported option '-fdebug-types-section' for target 'x86_64-apple-darwin'
 //
 // NOFDTS-NOT: "-mllvm" "-generate-type-units"
+// NOFDTSE-NOT: error: unsupported option '-fdebug-types-section' for target 'x86_64-apple-darwin'
 //
 // CI: "-dwarf-column-info"
 //
___
cfe-commits mailing list
cfe-commits@lists.llvm.o

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

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


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] D41102: Setup clang-doc frontend framework

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: tools/clang-doc/ClangDoc.h:40
+
+  void ParseUnattachedComments();
+  bool ParseNewDecl(const NamedDecl *D);

I know it's confusing given the amount of existing code that uses 
UpperCamelCase for functions, but I think that (as this is new code) we'd want 
to stay close to the style guide and use lowerCamelCase where we can. 



Comment at: tools/clang-doc/ClangDocReporter.h:35
+
+struct StringPair {
+  std::string Key;

Do you still need this?



Comment at: tools/clang-doc/tool/ClangDocMain.cpp:43
+  doc::OutFormat EmitFormat;
+  EmitLLVM ? EmitFormat = clang::doc::OutFormat::LLVM
+   : EmitFormat = clang::doc::OutFormat::YAML;

I'm curious if there's a particular reason that you seems to prefer

```
EmitLLVM ? EmitFormat = clang::doc::OutFormat::LLVM
 : EmitFormat = clang::doc::OutFormat::YAML;
```

over

```
EmitFormat = EmitLLVM ? clang::doc::OutFormat::LLVM
  : clang::doc::OutFormat::YAML;
```


https://reviews.llvm.org/D41102



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

[PATCH] D41102: Setup clang-doc frontend framework

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I don't know what basis is used to differentiate between the two, but should 
this be part of clang tools or clang-tools-extra?


https://reviews.llvm.org/D41102



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

[PATCH] D39834: [clang] -foptimization-record-file= should imply -fsave-optimization-record

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Thanks Dmitry, this LGTM!

PS: Let me know if you don't have commit access and want me to commit it for 
you.


https://reviews.llvm.org/D39834



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

[PATCH] D39834: [clang] -foptimization-record-file= should imply -fsave-optimization-record

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC321090: [clang] -foptimization-record-file= should imply 
-fsave-optimization-record (authored by JDevlieghere, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D39834

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/opt-record.c


Index: test/Driver/opt-record.c
===
--- test/Driver/opt-record.c
+++ test/Driver/opt-record.c
@@ -9,6 +9,8 @@
 // RUN: %clang -### -S -fsave-optimization-record -x cuda -nocudainc 
-nocudalib %s 2>&1 | FileCheck %s -check-prefix=CHECK-NO-O 
-check-prefix=CHECK-CUDA-DEV
 // RUN: %clang -### -fsave-optimization-record -x cuda -nocudainc -nocudalib 
%s 2>&1 | FileCheck %s -check-prefix=CHECK-NO-O -check-prefix=CHECK-CUDA-DEV
 // RUN: %clang -### -S -o FOO -fsave-optimization-record 
-foptimization-record-file=BAR.txt %s 2>&1 | FileCheck %s -check-prefix=CHECK-EQ
+// RUN: %clang -### -S -o FOO -foptimization-record-file=BAR.txt %s 2>&1 | 
FileCheck %s -check-prefix=CHECK-EQ
+// RUN: %clang -### -S -o FOO -foptimization-record-file=BAR.txt 
-fno-save-optimization-record %s 2>&1 | FileCheck %s 
--check-prefix=CHECK-FOPT-DISABLE
 
 // CHECK: "-cc1"
 // CHECK: "-opt-record-file" "FOO.opt.yaml"
@@ -20,3 +22,4 @@
 // CHECK-EQ: "-cc1"
 // CHECK-EQ: "-opt-record-file" "BAR.txt"
 
+// CHECK-FOPT-DISABLE-NOT: "-fno-save-optimization-record"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4389,6 +4389,7 @@
 CmdArgs.push_back("-fapple-pragma-pack");
 
   if (Args.hasFlag(options::OPT_fsave_optimization_record,
+   options::OPT_foptimization_record_file_EQ,
options::OPT_fno_save_optimization_record, false)) {
 CmdArgs.push_back("-opt-record-file");
 


Index: test/Driver/opt-record.c
===
--- test/Driver/opt-record.c
+++ test/Driver/opt-record.c
@@ -9,6 +9,8 @@
 // RUN: %clang -### -S -fsave-optimization-record -x cuda -nocudainc -nocudalib %s 2>&1 | FileCheck %s -check-prefix=CHECK-NO-O -check-prefix=CHECK-CUDA-DEV
 // RUN: %clang -### -fsave-optimization-record -x cuda -nocudainc -nocudalib %s 2>&1 | FileCheck %s -check-prefix=CHECK-NO-O -check-prefix=CHECK-CUDA-DEV
 // RUN: %clang -### -S -o FOO -fsave-optimization-record -foptimization-record-file=BAR.txt %s 2>&1 | FileCheck %s -check-prefix=CHECK-EQ
+// RUN: %clang -### -S -o FOO -foptimization-record-file=BAR.txt %s 2>&1 | FileCheck %s -check-prefix=CHECK-EQ
+// RUN: %clang -### -S -o FOO -foptimization-record-file=BAR.txt -fno-save-optimization-record %s 2>&1 | FileCheck %s --check-prefix=CHECK-FOPT-DISABLE
 
 // CHECK: "-cc1"
 // CHECK: "-opt-record-file" "FOO.opt.yaml"
@@ -20,3 +22,4 @@
 // CHECK-EQ: "-cc1"
 // CHECK-EQ: "-opt-record-file" "BAR.txt"
 
+// CHECK-FOPT-DISABLE-NOT: "-fno-save-optimization-record"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4389,6 +4389,7 @@
 CmdArgs.push_back("-fapple-pragma-pack");
 
   if (Args.hasFlag(options::OPT_fsave_optimization_record,
+   options::OPT_foptimization_record_file_EQ,
options::OPT_fno_save_optimization_record, false)) {
 CmdArgs.push_back("-opt-record-file");
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: aprantl, dexonsmith, dblaikie, labath.

As brought up during the discussion of the DWARF5 accelerator tables,
there is currently no way to associate Objective-C methods with the
interface they belong to, other than the .apple_objc accelerator table.

After due consideration we came to the conclusion that it makes more
sense to follow Pavel's suggestion of just emitting this information in
the .debug_info section. One concern was that categories were
emitted in the .apple_names as well, but it turns out that LLDB doesn't
rely on the accelerator tables for this information.

This patch changes the codegen behavior to emit subprograms for
structure types, like we do for C++. This will result in the
DW_TAG_subprogram being nested as a child under its
DW_TAG_structure_type. This behavior is only enabled for DWARF5 and
later, so we can have a unique code path in LLDB with regards to
obtaining the class methods.

For more background please refer to the discussion on the mailing list:
http://lists.llvm.org/pipermail/llvm-dev/2018-June/123986.html


Repository:
  rC Clang

https://reviews.llvm.org/D48241

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGenObjC/debug-info-synthesis.m

Index: clang/test/CodeGenObjC/debug-info-synthesis.m
===
--- clang/test/CodeGenObjC/debug-info-synthesis.m
+++ clang/test/CodeGenObjC/debug-info-synthesis.m
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+
 # 1 "foo.m" 1
 # 1 "foo.m" 2
 # 1 "./foo.h" 1
@@ -30,8 +32,13 @@
   }
 }
 
+// DWARF5: ![[STRUCT:.*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
 // CHECK: ![[FILE:.*]] = !DIFile(filename: "{{[^"]+}}foo.h"
-// CHECK: !DISubprogram(name: "-[Foo setDict:]"
+// DWARF5: !DISubprogram(name: "-[Foo setDict:]"
+// DWARF5-SAME:  scope: ![[STRUCT]],
+// DWARF5-SAME:  line: 8,
+// DWARF5-SAME:  isLocal: true, isDefinition: false
+// CHECK: distinct !DISubprogram(name: "-[Foo setDict:]"
 // CHECK-SAME:  file: ![[FILE]],
 // CHECK-SAME:  line: 8,
 // CHECK-SAME:  isLocal: true, isDefinition: true
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,27 @@
   /// Cache of previously constructed interfaces which may change.
   llvm::SmallVector ObjCInterfaceCache;
 
+  struct ObjCMethodCacheEntry {
+struct MethodData {
+  const ObjCMethodDecl *MD;
+  llvm::DISubprogram *DIMethodDecl;
+  MethodData(const ObjCMethodDecl *MD, llvm::DISubprogram *DIMethodDecl)
+  : MD(MD), DIMethodDecl(DIMethodDecl) {}
+};
+
+// Keep track of the interface so we can update its elements with its
+// methods.
+llvm::DICompositeType *DIInterfaceDecl;
+std::vector Methods;
+
+ObjCMethodCacheEntry(llvm::DICompositeType *DIInterfaceDecl = nullptr)
+: DIInterfaceDecl(DIInterfaceDecl) {}
+  };
+
+  /// Cache of forward declarations for method
+  llvm::DenseMap
+  ObjCMethodCache;
+
   /// Cache of references to clang modules and precompiled headers.
   llvm::DenseMap ModuleCache;
 
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2231,6 +2231,13 @@
   Mod ? Mod : Unit, ID->getName(), DefUnit, Line, Size, Align, Flags,
   nullptr, llvm::DINodeArray(), RuntimeLang);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Remember the DICompositeType in the ObjCMethodCache.
+assert(ObjCMethodCache[ID].DIInterfaceDecl == nullptr ||
+   ObjCMethodCache[ID].DIInterfaceDecl == RealDecl);
+ObjCMethodCache[ID].DIInterfaceDecl = RealDecl;
+  }
+
   QualType QTy(Ty, 0);
   TypeCache[QTy.getAsOpaquePtr()].reset(RealDecl);
 
@@ -3346,6 +3353,21 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF5, we create declarations for the interface's
+// methods.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+  const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+  llvm::DISubprogram *FD = DBuilder.createFunction(
+  ObjCMethodCache[ID].DIInterfaceDecl, Name, LinkageName, Unit, LineNo,
+  getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+  false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+  TParamsArray.get());
+  DBuilder.finalizeSubprogram(FD);
+  ObjCMethodC

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 151580.
JDevlieghere marked 5 inline comments as done.
JDevlieghere added a comment.

CR feedback


https://reviews.llvm.org/D48241

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGenObjC/debug-info-synthesis.m

Index: clang/test/CodeGenObjC/debug-info-synthesis.m
===
--- clang/test/CodeGenObjC/debug-info-synthesis.m
+++ clang/test/CodeGenObjC/debug-info-synthesis.m
@@ -1,4 +1,7 @@
 // RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+// RUN: %clang_cc1 -dwarf-version=4 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF4
+
 # 1 "foo.m" 1
 # 1 "foo.m" 2
 # 1 "./foo.h" 1
@@ -30,8 +33,14 @@
   }
 }
 
+// CHECK: ![[STRUCT:.*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
 // CHECK: ![[FILE:.*]] = !DIFile(filename: "{{[^"]+}}foo.h"
-// CHECK: !DISubprogram(name: "-[Foo setDict:]"
+// DWARF5: !DISubprogram(name: "-[Foo setDict:]"
+// DWARF5-SAME:  scope: ![[STRUCT]],
+// DWARF5-SAME:  line: 8,
+// DWARF5-SAME:  isLocal: true, isDefinition: false
+// DWARF4-NOT: !DISubprogram(name: "-[Foo setDict:]"{{.*}}isDefinition: false
+// CHECK: distinct !DISubprogram(name: "-[Foo setDict:]"
 // CHECK-SAME:  file: ![[FILE]],
 // CHECK-SAME:  line: 8,
 // CHECK-SAME:  isLocal: true, isDefinition: true
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,25 @@
   /// Cache of previously constructed interfaces which may change.
   llvm::SmallVector ObjCInterfaceCache;
 
+  struct ObjCMethodCacheEntry {
+struct MethodData {
+  const ObjCMethodDecl *MD;
+  llvm::DISubprogram *DIMethodDecl;
+};
+
+// Keep track of the interface so we can update its elements with its
+// methods.
+llvm::DICompositeType *DIInterfaceDecl;
+std::vector Methods;
+
+ObjCMethodCacheEntry(llvm::DICompositeType *DIInterfaceDecl = nullptr)
+: DIInterfaceDecl(DIInterfaceDecl) {}
+  };
+
+  /// Cache of forward declarations for method
+  llvm::DenseMap
+  ObjCMethodCache;
+
   /// Cache of references to clang modules and precompiled headers.
   llvm::DenseMap ModuleCache;
 
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2231,6 +2231,13 @@
   Mod ? Mod : Unit, ID->getName(), DefUnit, Line, Size, Align, Flags,
   nullptr, llvm::DINodeArray(), RuntimeLang);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Remember the DICompositeType in the ObjCMethodCache.
+assert(ObjCMethodCache[ID].DIInterfaceDecl == nullptr ||
+   ObjCMethodCache[ID].DIInterfaceDecl == RealDecl);
+ObjCMethodCache[ID].DIInterfaceDecl = RealDecl;
+  }
+
   QualType QTy(Ty, 0);
   TypeCache[QTy.getAsOpaquePtr()].reset(RealDecl);
 
@@ -3346,6 +3353,21 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+  const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+  llvm::DISubprogram *FD = DBuilder.createFunction(
+  ObjCMethodCache[ID].DIInterfaceDecl, Name, LinkageName, Unit, LineNo,
+  getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+  false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+  TParamsArray.get());
+  DBuilder.finalizeSubprogram(FD);
+  ObjCMethodCache[ID].Methods.push_back({OMD, FD});
+}
+  }
+
   // Push the function onto the lexical block stack.
   LexicalBlockStack.emplace_back(SP);
 
@@ -4213,6 +4235,26 @@
 DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
   }
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto p : ObjCMethodCache) {
+  llvm::DICompositeType *RealDecl = p.second.DIInterfaceDecl;
+  if (!RealDecl)
+continue;
+  if (p.second.Methods.empty())
+continue;
+
+  SmallVector EltTys;
+  EltTys.append(RealDecl->getElements().begin(),
+RealDecl->getElements().end());
+  for (auto &M : p.second.Methods) {
+EltTys.push_back(M.DIMethodDecl);
+  }
+  llvm::DINodeArray Elements = DBuilder.getOrCreateArray(EltTys);
+  DBuilder.replaceArrays(RealDecl, E

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D48241#1134240, @dblaikie wrote:

> Not sure I should get too involved in this discussion, knowing so little 
> about Objective C - but if it seems sensible, could you provide some examples 
> (just in comments/description in this code review) of what the DWARF used to 
> look like and what it looks like after this change?


After this change we emit a forward declaration as a child of the 
DW_TAG_structure_type. The definition (below) remains unchanged.

  0x0027:   DW_TAG_structure_type
  DW_AT_APPLE_objc_complete_type  (true)
  DW_AT_name  ()
  DW_AT_byte_size (0x04)
  DW_AT_decl_file ("cat.m")
  DW_AT_decl_line (1)
  DW_AT_APPLE_runtime_class   (0x10)
  
  0x002d: DW_TAG_member
DW_AT_name()
DW_AT_type(0x007b "base ")
DW_AT_decl_file   ("cat.m")
DW_AT_decl_line   (2)
DW_AT_data_member_location(0x00)
DW_AT_accessibility   (DW_ACCESS_protected)
  
  0x0037: DW_TAG_subprogram
DW_AT_name()
DW_AT_decl_file   ("cat.m")
DW_AT_decl_line   (10)
DW_AT_prototyped  (true)
DW_AT_type(0x007b "base ")
DW_AT_declaration (true)
  
  0x003f:   DW_TAG_formal_parameter
  DW_AT_type  (0x007f "structure *")
  DW_AT_artificial(true)
  
  0x0044:   DW_TAG_formal_parameter
  DW_AT_type  (0x0084 "structure *")
  DW_AT_artificial(true)
  
  0x0049:   NULL
  ...
  0x007a: NULL
  ...
  0x00b5:   DW_TAG_subprogram
  DW_AT_low_pc(0x)
  DW_AT_high_pc   (0x001c)
  DW_AT_frame_base(DW_OP_reg6 RBP)
  DW_AT_object_pointer(0x00cf)
  DW_AT_name  ()
  DW_AT_decl_file ("cat.m")
  DW_AT_decl_line (10)
  DW_AT_prototyped(true)
  DW_AT_type  (0x007b "base ")
  
  0x00cf: DW_TAG_formal_parameter
DW_AT_location(DW_OP_fbreg -8)
DW_AT_name()
DW_AT_type(0x00b0 "structure *")
DW_AT_artificial  (true)
  
  0x00d8: DW_TAG_formal_parameter
DW_AT_location(DW_OP_fbreg -16)
DW_AT_name()
DW_AT_type(0x0195 "structure *")
DW_AT_artificial  (true)
  
  0x00e1: NULL



> Does this address the discoverability that's being discussed in the llvm-dev 
> thread? There were concerns there about having to search through all the 
> instances of a type to find all of its functions - I imagine, since Objective 
> C classes aren't closed (if I understand correctly) that would still be a 
> concern here? If it is, what is the benefit/tradeoff of this change (if it 
> doesn't address the discoverability/still requires the Objective C 
> accelerator table)?

Following this approach we have the exact same problem (and I believe we could 
use the same solution). The motivation for this change is that there's no clean 
way to implement the .apple_objc accelerator table using the debug_names, 
because it's conceptually different from the other tables. It maps an interface 
name to the DIEs of its methods, as opposed to all the other tables that map a 
name to their corresponding DIE. The only way this could work is as a separate 
table, because otherwise you'd have to ensure, for every entry, if it's a 
"regular" one, which obviously violates the standard. Putting it in a separate 
column is also a bad idea, because that means the column is present for all the 
entries, including the ones that don't need it.




Comment at: clang/lib/CodeGen/CGDebugInfo.h:111
+// methods.
+llvm::DICompositeType *DIInterfaceDecl;
+std::vector Methods;

aprantl wrote:
> Isn't the interface already the key in the DenseMap?
That's the `ObjCInterfaceDecl` while this is the `DICompositeType`. 


https://reviews.llvm.org/D48241



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

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 151591.
JDevlieghere added a comment.

- Use type cache &
- Simplify method cache struct
- Add custom test that verifies category methods are emitted


https://reviews.llvm.org/D48241

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGenObjC/debug-info-category.m

Index: clang/test/CodeGenObjC/debug-info-category.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/debug-info-category.m
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+// RUN: %clang_cc1 -dwarf-version=4 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF4
+
+@interface Foo {
+  int integer;
+}
+
+- (int)integer;
+- (id)integer:(int)_integer;
+@end
+
+@implementation Foo
+- (int)integer {
+  return integer;
+}
+
+- (id)integer:(int)_integer {
+  integer = _integer;
+  return self;
+}
+@end
+
+@interface Foo (Bar)
+- (id)add:(Foo *)addend;
+@end
+
+@implementation Foo (Bar)
+- (id)add:(Foo *)addend {
+  return [self integer:[self integer] + [addend integer]];
+}
+@end
+
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
+
+// DWARF5: = !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: false
+// DWARF5: = !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: false
+// DWARF5: = !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: false
+
+// DWARF4-NOT: = !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: false
+// DWARF4-NOT: = !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: false
+// DWARF4-NOT: = !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: false
+
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: true
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,15 @@
   /// Cache of previously constructed interfaces which may change.
   llvm::SmallVector ObjCInterfaceCache;
 
+  struct ObjCMethodCacheEntry {
+const ObjCMethodDecl *MD;
+llvm::DISubprogram *DIMethodDecl;
+  };
+
+  /// Cache of forward declarations for method
+  llvm::DenseMap>
+  ObjCMethodCache;
+
   /// Cache of references to clang modules and precompiled headers.
   llvm::DenseMap ModuleCache;
 
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3346,6 +3346,26 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+  const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+  QualType QTy(ID->getTypeForDecl(), 0);
+  auto it = TypeCache.find(QTy.getAsOpaquePtr());
+  assert(it != TypeCache.end());
+  llvm::DICompositeType *InterfaceDecl =
+  cast(it->second);
+  llvm::DISubprogram *FD = DBuilder.createFunction(
+  InterfaceDecl, Name, LinkageName, Unit, LineNo,
+  getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+  false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+  TParamsArray.get());
+  DBuilder.finalizeSubprogram(FD);
+  ObjCMethodCache[ID].push_back({OMD, FD});
+}
+  }
+
   // Push the function onto the lexical block stack.
   LexicalBlockStack.emplace_back(SP);
 
@@ -4213,6 +4233,31 @@
 DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
   }
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto p : ObjCMethodCache) {
+  if (p.second.empty())
+continue;
+
+  QualType QTy(p.first->getTypeForDecl(), 0);
+  auto it = TypeCache.find(QTy.getAsOpaquePtr());
+  if (it == TypeCache.end())
+continue;
+
+  llvm::DICompositeType *InterfaceDecl =
+  cast(it->second);
+
+  SmallVector EltTys;
+  EltTys.append(InterfaceDecl->getElements().begin(),
+InterfaceDecl->getElements().end());
+  for (auto &M : p.second) {
+EltTys.push_back(M.DIMethodDecl);
+  }
+  llvm::DINodeArray Elements = DBuilder.getOrCreateArray(EltTys);
+  DBuilder.replaceArrays(InterfaceDecl, Elements);
+}
+  }
+
   for (auto p : ReplaceMap) {
 assert(p.second);
 auto *Ty = cast(p.second);

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 151596.
JDevlieghere marked 3 inline comments as done.
JDevlieghere added a comment.

- Feedback Adrian


https://reviews.llvm.org/D48241

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGenObjC/debug-info-category.m

Index: clang/test/CodeGenObjC/debug-info-category.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/debug-info-category.m
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+// RUN: %clang_cc1 -dwarf-version=4 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF4
+
+@interface Foo {
+  int integer;
+}
+
+- (int)integer;
+- (id)integer:(int)_integer;
+@end
+
+@implementation Foo
+- (int)integer {
+  return integer;
+}
+
+- (id)integer:(int)_integer {
+  integer = _integer;
+  return self;
+}
+@end
+
+@interface Foo (Bar)
+- (id)add:(Foo *)addend;
+@end
+
+@implementation Foo (Bar)
+- (id)add:(Foo *)addend {
+  return [self integer:[self integer] + [addend integer]];
+}
+@end
+
+// 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) 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) add:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: true
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,15 @@
   /// Cache of previously constructed interfaces which may change.
   llvm::SmallVector ObjCInterfaceCache;
 
+  struct ObjCMethodCacheEntry {
+const ObjCMethodDecl *MD;
+llvm::DISubprogram *DIMethodDecl;
+  };
+
+  /// Cache of forward declarations for methods belonging to the interface.
+  llvm::DenseMap>
+  ObjCMethodCache;
+
   /// Cache of references to clang modules and precompiled headers.
   llvm::DenseMap ModuleCache;
 
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3346,6 +3346,26 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+  const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+  QualType QTy(ID->getTypeForDecl(), 0);
+  auto it = TypeCache.find(QTy.getAsOpaquePtr());
+  assert(it != TypeCache.end());
+  llvm::DICompositeType *InterfaceDecl =
+  cast(it->second);
+  llvm::DISubprogram *FD = DBuilder.createFunction(
+  InterfaceDecl, Name, LinkageName, Unit, LineNo,
+  getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+  false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+  TParamsArray.get());
+  DBuilder.finalizeSubprogram(FD);
+  ObjCMethodCache[ID].push_back({OMD, FD});
+}
+  }
+
   // Push the function onto the lexical block stack.
   LexicalBlockStack.emplace_back(SP);
 
@@ -4213,6 +4233,30 @@
 DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
   }
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto p : ObjCMethodCache) {
+  if (p.second.empty())
+continue;
+
+  QualType QTy(p.first->getTypeForDecl(), 0);
+  auto it = TypeCache.find(QTy.getAsOpaquePtr());
+  if (it == TypeCache.end())
+continue;
+
+  llvm::DICompositeType *InterfaceDecl =
+  cast(it->second);
+
+  SmallVector EltTys;
+  auto &Elements = InterfaceDecl->getElements();
+  EltTys.append(Elements.begin(), Elements.end());
+  for (auto &M : p.second)
+EltTys.push_back(M.DIMethodDecl);
+  llvm::DINodeArray Elements = DBuilder.getOrCreateArray(EltTys);
+  DBuilder.replaceArrays(InterfaceDecl, Elements);
+}
+  }
+
   for (auto p : Replace

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D48241#1134985, @labath wrote:

> This is not true. (Unlike the .apple_*** tables, ) .debug_names allows you to 
> specify a different schema for every entry in the acelerator table. The 
> schema is specifing using abbreviations in much the same way as .debug_abbrev 
> specifies the schema for .debug_info. So you could easily have one 
> abbreviation for regular classes, and a different one for objc interfaces. 
> Currently, our .debug_names generator does not support these kinds of 
> heterogeneous tables, but that's simply because I had no use for it. It could 
> be added if necessary (though it will require some 
> generalization/refactoring). OTOH, our consumer should already be able to 
> handle these kinds of tables as input.


Indeed, you are correct, I was thinking about the Apple structure with the 
atoms and forgot about the abbreviation in each entries for DWARF. Thanks for 
pointing this out!


https://reviews.llvm.org/D48241



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

[PATCH] D53200: [OpenCL] Fix serialization of OpenCLExtensionDecls

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Can you add a test case please?


Repository:
  rC Clang

https://reviews.llvm.org/D53200



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

[PATCH] D53200: [OpenCL] Fix serialization of OpenCLExtensionDecls

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

The patch looks fine but since I don't know much about opencl I'll leave the 
LGTM to someone that actually knows this code.




Comment at: test/Headers/opencl-pragma-extension-begin.h:1
+
+#pragma OPENCL EXTENSION cl_my_ext : begin

Is this newline intentional? 


https://reviews.llvm.org/D53200



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

[PATCH] D57411: [ModuleDependencyCollector] Use llvm::sys::fs::real_path

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: bruno.

Use the real_path from llvm::sys::fs::real_path instead of having a custom 
implementation.


Repository:
  rC Clang

https://reviews.llvm.org/D57411

Files:
  clang/lib/Frontend/ModuleDependencyCollector.cpp


Index: clang/lib/Frontend/ModuleDependencyCollector.cpp
===
--- clang/lib/Frontend/ModuleDependencyCollector.cpp
+++ clang/lib/Frontend/ModuleDependencyCollector.cpp
@@ -98,24 +98,6 @@
 
 }
 
-// TODO: move this to Support/Path.h and check for HAVE_REALPATH?
-static bool real_path(StringRef SrcPath, SmallVectorImpl &RealPath) {
-#ifdef LLVM_ON_UNIX
-  char CanonicalPath[PATH_MAX];
-
-  // TODO: emit a warning in case this fails...?
-  if (!realpath(SrcPath.str().c_str(), CanonicalPath))
-return false;
-
-  SmallString<256> RPath(CanonicalPath);
-  RealPath.swap(RPath);
-  return true;
-#else
-  // FIXME: Add support for systems without realpath.
-  return false;
-#endif
-}
-
 void ModuleDependencyCollector::attachToASTReader(ASTReader &R) {
   R.addListener(llvm::make_unique(*this));
 }
@@ -130,7 +112,7 @@
 static bool isCaseSensitivePath(StringRef Path) {
   SmallString<256> TmpDest = Path, UpperDest, RealDest;
   // Remove component traversals, links, etc.
-  if (!real_path(Path, TmpDest))
+  if (llvm::sys::fs::real_path(Path, TmpDest))
 return true; // Current default value in vfs.yaml
   Path = TmpDest;
 
@@ -140,7 +122,7 @@
   // already expects when sensitivity isn't setup.
   for (auto &C : Path)
 UpperDest.push_back(toUppercase(C));
-  if (real_path(UpperDest, RealDest) && Path.equals(RealDest))
+  if (!llvm::sys::fs::real_path(UpperDest, RealDest) && Path.equals(RealDest))
 return false;
   return true;
 }
@@ -186,7 +168,7 @@
   // Computing the real path is expensive, cache the search through the
   // parent path directory.
   if (DirWithSymLink == SymLinkMap.end()) {
-if (!real_path(Dir, RealPath))
+if (llvm::sys::fs::real_path(Dir, RealPath))
   return false;
 SymLinkMap[Dir] = RealPath.str();
   } else {


Index: clang/lib/Frontend/ModuleDependencyCollector.cpp
===
--- clang/lib/Frontend/ModuleDependencyCollector.cpp
+++ clang/lib/Frontend/ModuleDependencyCollector.cpp
@@ -98,24 +98,6 @@
 
 }
 
-// TODO: move this to Support/Path.h and check for HAVE_REALPATH?
-static bool real_path(StringRef SrcPath, SmallVectorImpl &RealPath) {
-#ifdef LLVM_ON_UNIX
-  char CanonicalPath[PATH_MAX];
-
-  // TODO: emit a warning in case this fails...?
-  if (!realpath(SrcPath.str().c_str(), CanonicalPath))
-return false;
-
-  SmallString<256> RPath(CanonicalPath);
-  RealPath.swap(RPath);
-  return true;
-#else
-  // FIXME: Add support for systems without realpath.
-  return false;
-#endif
-}
-
 void ModuleDependencyCollector::attachToASTReader(ASTReader &R) {
   R.addListener(llvm::make_unique(*this));
 }
@@ -130,7 +112,7 @@
 static bool isCaseSensitivePath(StringRef Path) {
   SmallString<256> TmpDest = Path, UpperDest, RealDest;
   // Remove component traversals, links, etc.
-  if (!real_path(Path, TmpDest))
+  if (llvm::sys::fs::real_path(Path, TmpDest))
 return true; // Current default value in vfs.yaml
   Path = TmpDest;
 
@@ -140,7 +122,7 @@
   // already expects when sensitivity isn't setup.
   for (auto &C : Path)
 UpperDest.push_back(toUppercase(C));
-  if (real_path(UpperDest, RealDest) && Path.equals(RealDest))
+  if (!llvm::sys::fs::real_path(UpperDest, RealDest) && Path.equals(RealDest))
 return false;
   return true;
 }
@@ -186,7 +168,7 @@
   // Computing the real path is expensive, cache the search through the
   // parent path directory.
   if (DirWithSymLink == SymLinkMap.end()) {
-if (!real_path(Dir, RealPath))
+if (llvm::sys::fs::real_path(Dir, RealPath))
   return false;
 SymLinkMap[Dir] = RealPath.str();
   } else {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57411: [ModuleDependencyCollector] Use llvm::sys::fs::real_path

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352605: [ModuleDependencyCollector] Use 
llvm::sys::fs::real_path (NFC) (authored by JDevlieghere, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57411?vs=184155&id=184250#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57411

Files:
  lib/Frontend/ModuleDependencyCollector.cpp


Index: lib/Frontend/ModuleDependencyCollector.cpp
===
--- lib/Frontend/ModuleDependencyCollector.cpp
+++ lib/Frontend/ModuleDependencyCollector.cpp
@@ -98,24 +98,6 @@
 
 }
 
-// TODO: move this to Support/Path.h and check for HAVE_REALPATH?
-static bool real_path(StringRef SrcPath, SmallVectorImpl &RealPath) {
-#ifdef LLVM_ON_UNIX
-  char CanonicalPath[PATH_MAX];
-
-  // TODO: emit a warning in case this fails...?
-  if (!realpath(SrcPath.str().c_str(), CanonicalPath))
-return false;
-
-  SmallString<256> RPath(CanonicalPath);
-  RealPath.swap(RPath);
-  return true;
-#else
-  // FIXME: Add support for systems without realpath.
-  return false;
-#endif
-}
-
 void ModuleDependencyCollector::attachToASTReader(ASTReader &R) {
   R.addListener(llvm::make_unique(*this));
 }
@@ -130,7 +112,7 @@
 static bool isCaseSensitivePath(StringRef Path) {
   SmallString<256> TmpDest = Path, UpperDest, RealDest;
   // Remove component traversals, links, etc.
-  if (!real_path(Path, TmpDest))
+  if (llvm::sys::fs::real_path(Path, TmpDest))
 return true; // Current default value in vfs.yaml
   Path = TmpDest;
 
@@ -140,7 +122,7 @@
   // already expects when sensitivity isn't setup.
   for (auto &C : Path)
 UpperDest.push_back(toUppercase(C));
-  if (real_path(UpperDest, RealDest) && Path.equals(RealDest))
+  if (!llvm::sys::fs::real_path(UpperDest, RealDest) && Path.equals(RealDest))
 return false;
   return true;
 }
@@ -186,7 +168,7 @@
   // Computing the real path is expensive, cache the search through the
   // parent path directory.
   if (DirWithSymLink == SymLinkMap.end()) {
-if (!real_path(Dir, RealPath))
+if (llvm::sys::fs::real_path(Dir, RealPath))
   return false;
 SymLinkMap[Dir] = RealPath.str();
   } else {


Index: lib/Frontend/ModuleDependencyCollector.cpp
===
--- lib/Frontend/ModuleDependencyCollector.cpp
+++ lib/Frontend/ModuleDependencyCollector.cpp
@@ -98,24 +98,6 @@
 
 }
 
-// TODO: move this to Support/Path.h and check for HAVE_REALPATH?
-static bool real_path(StringRef SrcPath, SmallVectorImpl &RealPath) {
-#ifdef LLVM_ON_UNIX
-  char CanonicalPath[PATH_MAX];
-
-  // TODO: emit a warning in case this fails...?
-  if (!realpath(SrcPath.str().c_str(), CanonicalPath))
-return false;
-
-  SmallString<256> RPath(CanonicalPath);
-  RealPath.swap(RPath);
-  return true;
-#else
-  // FIXME: Add support for systems without realpath.
-  return false;
-#endif
-}
-
 void ModuleDependencyCollector::attachToASTReader(ASTReader &R) {
   R.addListener(llvm::make_unique(*this));
 }
@@ -130,7 +112,7 @@
 static bool isCaseSensitivePath(StringRef Path) {
   SmallString<256> TmpDest = Path, UpperDest, RealDest;
   // Remove component traversals, links, etc.
-  if (!real_path(Path, TmpDest))
+  if (llvm::sys::fs::real_path(Path, TmpDest))
 return true; // Current default value in vfs.yaml
   Path = TmpDest;
 
@@ -140,7 +122,7 @@
   // already expects when sensitivity isn't setup.
   for (auto &C : Path)
 UpperDest.push_back(toUppercase(C));
-  if (real_path(UpperDest, RealDest) && Path.equals(RealDest))
+  if (!llvm::sys::fs::real_path(UpperDest, RealDest) && Path.equals(RealDest))
 return false;
   return true;
 }
@@ -186,7 +168,7 @@
   // Computing the real path is expensive, cache the search through the
   // parent path directory.
   if (DirWithSymLink == SymLinkMap.end()) {
-if (!real_path(Dir, RealPath))
+if (llvm::sys::fs::real_path(Dir, RealPath))
   return false;
 SymLinkMap[Dir] = RealPath.str();
   } else {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

The patch applies for me but has quite a few style violations. I'll fix those 
up before landing it.


https://reviews.llvm.org/D53263



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

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346601: Pass the function type instead of the return type to 
FunctionDecl::Create (authored by JDevlieghere, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53263?vs=170277&id=173542#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53263

Files:
  cfe/trunk/lib/AST/Decl.cpp
  cfe/trunk/lib/CodeGen/CGBlocks.cpp
  cfe/trunk/lib/CodeGen/CGBuiltin.cpp
  cfe/trunk/lib/CodeGen/CGNonTrivialStruct.cpp
  cfe/trunk/lib/CodeGen/CGObjC.cpp
  cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
  cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
  cfe/trunk/lib/Frontend/Rewrite/RewriteModernObjC.cpp
  cfe/trunk/test/CodeGenObjCXX/crash-function-type.mm

Index: cfe/trunk/lib/AST/Decl.cpp
===
--- cfe/trunk/lib/AST/Decl.cpp
+++ cfe/trunk/lib/AST/Decl.cpp
@@ -2652,6 +2652,7 @@
  StartLoc),
   DeclContext(DK), redeclarable_base(C), ODRHash(0),
   EndRangeLoc(NameInfo.getEndLoc()), DNLoc(NameInfo.getInfo()) {
+  assert(T.isNull() || T->isFunctionType());
   setStorageClass(S);
   setInlineSpecified(isInlineSpecified);
   setExplicitSpecified(false);
Index: cfe/trunk/lib/Frontend/Rewrite/RewriteModernObjC.cpp
===
--- cfe/trunk/lib/Frontend/Rewrite/RewriteModernObjC.cpp
+++ cfe/trunk/lib/Frontend/Rewrite/RewriteModernObjC.cpp
@@ -3099,10 +3099,9 @@
  SmallVectorImpl &MsgExprs,
  ObjCMethodDecl *Method) {
   // Now do the "normal" pointer to function cast.
-  QualType castType = getSimpleFunctionType(returnType, ArgTypes,
-Method ? Method->isVariadic()
-   : false);
-  castType = Context->getPointerType(castType);
+  QualType FuncType = getSimpleFunctionType(
+  returnType, ArgTypes, Method ? Method->isVariadic() : false);
+  QualType castType = Context->getPointerType(FuncType);
 
   // build type for containing the objc_msgSend_stret object.
   static unsigned stretCount=0;
@@ -3176,9 +3175,9 @@
 
   // AST for __Stretn(receiver, args).s;
   IdentifierInfo *ID = &Context->Idents.get(name);
-  FunctionDecl *FD = FunctionDecl::Create(*Context, TUDecl, SourceLocation(),
-  SourceLocation(), ID, castType,
-  nullptr, SC_Extern, false, false);
+  FunctionDecl *FD =
+  FunctionDecl::Create(*Context, TUDecl, SourceLocation(), SourceLocation(),
+   ID, FuncType, nullptr, SC_Extern, false, false);
   DeclRefExpr *DRE = new (Context) DeclRefExpr(FD, false, castType, VK_RValue,
SourceLocation());
   CallExpr *STCE = new (Context) CallExpr(*Context, DRE, MsgExprs,
Index: cfe/trunk/lib/CodeGen/CGObjC.cpp
===
--- cfe/trunk/lib/CodeGen/CGObjC.cpp
+++ cfe/trunk/lib/CodeGen/CGObjC.cpp
@@ -3249,29 +3249,32 @@
   ASTContext &C = getContext();
   IdentifierInfo *II
 = &CGM.getContext().Idents.get("__assign_helper_atomic_property_");
-  FunctionDecl *FD = FunctionDecl::Create(C,
-  C.getTranslationUnitDecl(),
-  SourceLocation(),
-  SourceLocation(), II, C.VoidTy,
-  nullptr, SC_Static,
-  false,
-  false);
 
+  QualType ReturnTy = C.VoidTy;
   QualType DestTy = C.getPointerType(Ty);
   QualType SrcTy = Ty;
   SrcTy.addConst();
   SrcTy = C.getPointerType(SrcTy);
 
+  SmallVector ArgTys;
+  ArgTys.push_back(DestTy);
+  ArgTys.push_back(SrcTy);
+  QualType FunctionTy = C.getFunctionType(ReturnTy, ArgTys, {});
+
+  FunctionDecl *FD = FunctionDecl::Create(
+  C, C.getTranslationUnitDecl(), SourceLocation(), SourceLocation(), II,
+  FunctionTy, nullptr, SC_Static, false, false);
+
   FunctionArgList args;
-  ImplicitParamDecl DstDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
-DestTy, ImplicitParamDecl::Other);
+  ImplicitParamDecl DstDecl(C, FD, SourceLocation(), /*Id=*/nullptr, DestTy,
+ImplicitParamDecl::Other);
   args.push_back(&DstDecl);
-  ImplicitParamDecl SrcDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
-SrcTy, ImplicitParamDecl::Other);
+  ImplicitParamDecl SrcDecl(C, FD, SourceLocation(), /*Id=*/nullptr, SrcTy,
+ImplicitParamDecl::Other);
   args.push_back(&SrcDecl);
 
   const CGFunctionInfo &FI =
-CGM.getTypes().arrangeBuiltinFunctionDeclaration(C.VoidTy, args);
+ 

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D53263#1294497, @kristina wrote:

> In https://reviews.llvm.org/D53263#1294488, @JDevlieghere wrote:
>
> > The patch applies for me but has quite a few style violations. I'll fix 
> > those up before landing it.
>
>
> Thank you and sorry for the trouble, my fork seems to have enough 
> modifications to some of these files to confuse `patch` and getting an 
> untainted checkout and integrating it for a single build/test run would be 
> troublesome to undo.


No worries! I usually have a pretty up-to-date checkout around (I have cronjob 
that pulls and builds overnight) which comes in handy in situations like this.


Repository:
  rL LLVM

https://reviews.llvm.org/D53263



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

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I reverted this because it broke the LLDB bots on GreenDragon: 
http://green.lab.llvm.org/green/view/LLDB/


Repository:
  rL LLVM

https://reviews.llvm.org/D54310



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

[PATCH] D55128: [CMake] Store path to vendor-specific headers in clang-headers target property

[Jonas Devlieghere via Phabricator via cfe-commits]

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

This makes sense to me. I'm don't know if there's a better property but I think 
this matches the intended use, so I think it is fine.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55128



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

[PATCH] D58941: [clang-format][docs][NFC] Fix example for Allman brace breaking style

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM. Good catch, left looks like K&R or something.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58941



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

[PATCH] D58930: Add XCOFF triple object format type for AIX

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1470
+  case Triple::XCOFF:
+// TODO: Falling through for XCOFF format for now.
+break;

This is confusing, you say fall through but you break? I would prefer a 
`llvm_unreachable("XCOFF not yet implemented");` here and elsewhere in this 
patch. 




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1486
+  case Triple::XCOFF:
+// TODO: Falling through for XCOFF format for now.
+break;

See previous comment.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4410
+  case llvm::Triple::XCOFF:
+llvm_unreachable("to be determined for XCOFF format");
   case llvm::Triple::COFF:

See previous comment.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2079
+  if (log)
+log->Printf("sorry: unimplemented for XCOFF");
+  return false;

jasonliu wrote:
> apaprocki wrote:
> > No need to be `sorry:` :) This should probably just say `error: XCOFF is 
> > unimplemented` to be more direct in case anything is expecting "error:" in 
> > the output.
> Sure. Will address in next revision.
Just bundle this with the WASM case, the error message is correct for both.



Comment at: llvm/lib/MC/MCContext.cpp:165
+case MCObjectFileInfo::IsXCOFF:
+  // TODO: Need to implement class MCSymbolXCOFF.
+  break;

See previous comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58930



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

[PATCH] D58930: Add XCOFF triple object format type for AIX

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2079
+  if (log)
+log->Printf("sorry: unimplemented for XCOFF");
+  return false;

jasonliu wrote:
> JDevlieghere wrote:
> > jasonliu wrote:
> > > apaprocki wrote:
> > > > No need to be `sorry:` :) This should probably just say `error: XCOFF 
> > > > is unimplemented` to be more direct in case anything is expecting 
> > > > "error:" in the output.
> > > Sure. Will address in next revision.
> > Just bundle this with the WASM case, the error message is correct for both.
> I think they are different. 
> The error message for WASM seems to suggest that it will never ever get 
> supported on WASM. 
> But it is not the case for XCOFF, we want to indicate that it is not 
> implemented yet.  
I don't think the error message suggests that at all, and it's definitely not 
true. At this point neither XCOFF nor WASM is supported, and that's exactly 
what the log message says.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58930



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

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: lib/AST/ASTImporter.cpp:1951
+  // something we're trying to import while completin ToEnum
+  Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum);
+

```
if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum)) 
  if (auto *ToOriginEnum = dyn_cast(ToOrigin))
ToEnum = ToOriginEnum;
```


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

https://reviews.llvm.org/D59845



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

[PATCH] D60108: [os_log] Mark os_log_helper `nounwind`

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D60108



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

[PATCH] D51329: [Attribute/Diagnostics] Print macro if definition is an attribute declaration

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I think this is change is causing an assertion to be hit:

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/25103/consoleFull#-239233992d489585b-5106-414a-ac11-3ff90657619c
http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/56122/console

Can you have a look?


Repository:
  rC Clang

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

https://reviews.llvm.org/D51329



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

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I've reverted this in rL360192  because it 
breaks stage 2 builds on GreenDragon. Please see the commit message and the 
inline comment for more details.




Comment at: lib/Headers/CMakeLists.txt:36
   bmiintrin.h
+  openmp_wrappers/math.h
+  openmp_wrappers/cmath

This doesn't do what you think it would do. The files are copied into the root 
of the resource directory, which causes stage 2 build failures on GreenDragon. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D61399



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

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: lib/Headers/CMakeLists.txt:36
   bmiintrin.h
+  openmp_wrappers/math.h
+  openmp_wrappers/cmath

hfinkel wrote:
> JDevlieghere wrote:
> > This doesn't do what you think it would do. The files are copied into the 
> > root of the resource directory, which causes stage 2 build failures on 
> > GreenDragon. 
> Can you provide a link to the failure log? Is the problem that the files are 
> not copied into their subdirectory?
Correct. There's not much to see, but here's a build that fails because of 
this: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/25557/console

```
CMake Error at cmake/modules/HandleLLVMOptions.cmake:497 (message):
  LLVM_ENABLE_MODULES is not supported by this compiler
```
The cmake log output, which I grabbed from the bot is more descriptive:
```
Performing C++ SOURCE FILE Test CXX_SUPPORTS_MODULES failed with the following 
output:
Change Dir: 
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/CMakeFiles/CMakeTmp

Run Build Command:"/usr/local/bin/ninja" "cmTC_0dd11"
[1/2] Building CXX object CMakeFiles/cmTC_0dd11.dir/src.cxx.o
FAILED: CMakeFiles/cmTC_0dd11.dir/src.cxx.o
/Users/buildslave/jenkins/workspace/lldb-cmake/host-compiler/bin/clang++
-fPIC -fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -std=c++11 -DCXX_SUPPORTS_MODULES  
-Werror=unguarded-availability-new -fmodules 
-fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/module.cache
 -fcxx-modules -isysroot 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk
 -mmacosx-version-min=10.9 -o CMakeFiles/cmTC_0dd11.dir/src.cxx.o -c src.cxx
While building module 'Darwin' imported from 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/assert.h:42:
While building module 'std' imported from 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/tgmath.h:27:
In file included from :2:
/Users/buildslave/jenkins/workspace/lldb-cmake/host-compiler/bin/../include/c++/v1/ctype.h:38:15:
 fatal error: cyclic dependency in module 'Darwin': Darwin -> std -> Darwin
#include_next 
  ^
While building module 'Darwin' imported from 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/assert.h:42:
In file included from :89:
In file included from 
/Users/buildslave/jenkins/workspace/lldb-cmake/host-compiler/lib/clang/9.0.0/include/tgmath.h:21:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/tgmath.h:27:10:
 fatal error: could not build module 'std'
#include 
 ^
In file included from src.cxx:2:
In file included from 
/Users/buildslave/jenkins/workspace/lldb-cmake/host-compiler/bin/../include/c++/v1/cassert:20:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/assert.h:42:10:
 fatal error: could not build module 'Darwin'
#include 
 ^
3 errors generated.
ninja: build stopped: subcommand failed.

Source file was:
#undef NDEBUG
   #include 
   #define NDEBUG
   #include 
   int main() { assert(this code is not compiled); }
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D61399



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

[PATCH] D49549: Change 'clang-test' to 'check-clang' on the hacking webpage

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Hey Arnaud, let me know if you want me to commit this for you.


Repository:
  rC Clang

https://reviews.llvm.org/D49549



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

[PATCH] D52058: Add Parameters to DW_AT_name Attribute of Template Variables

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Generally this looks good, but I'd like for the other to have a look first (at 
this and the other patch) before accepting.




Comment at: lib/CodeGen/CGDebugInfo.cpp:1783
+  if (auto *TS = dyn_cast(VL)) {
+if (TS->getSpecializedTemplateOrPartial()
+.is()) {

Might be nice to add a comment here saying what you're doing in this block and 
below. Looks like the top one is for partial specialization and the bottom one 
for the general case?



Comment at: lib/CodeGen/CGDebugInfo.cpp:1783
+  if (auto *TS = dyn_cast(VL)) {
+if (TS->getSpecializedTemplateOrPartial()
+.is()) {

JDevlieghere wrote:
> Might be nice to add a comment here saying what you're doing in this block 
> and below. Looks like the top one is for partial specialization and the 
> bottom one for the general case?
I also suggest to extract `TS->getSpecializedTemplateOrPartial()` into a 
variable to make this a little less dense.


Repository:
  rC Clang

https://reviews.llvm.org/D52058



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

[PATCH] D52058: Add Parameters to DW_AT_name Attribute of Template Variables

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D52058



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

[PATCH] D57976: -gmodules: Don't emit incomplete breadcrumbs pointing to nonexistant PCM files.

[Jonas Devlieghere via Phabricator via cfe-commits]

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

This makes sense to me, just one nit.




Comment at: lib/CodeGen/CGDebugInfo.cpp:2304
+assert((!M || (M->Name == CGM.getLangOpts().ModuleName)) &&
+   "clang module without ASTFile was not specified by -fmodule-name");
+

Maybe it's just me but I had to reread this sentence a few times to be sure 
what you meant by the double negation. Maybe `clang module without ASTFile must 
be specified by -fmodule-name` is easier to grok.


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

https://reviews.llvm.org/D57976



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

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: bruno, vsapsai.
Herald added a project: clang.

For reproducers in LLDB we want to hook up into the existing clang 
infrastructure. To make that happen we need to be able to override the 
ModuleDependencyCollector's methods.


Repository:
  rC Clang

https://reviews.llvm.org/D58072

Files:
  clang/include/clang/Frontend/Utils.h


Index: clang/include/clang/Frontend/Utils.h
===
--- clang/include/clang/Frontend/Utils.h
+++ clang/include/clang/Frontend/Utils.h
@@ -145,18 +145,18 @@
   ~ModuleDependencyCollector() override { writeFileMap(); }
 
   StringRef getDest() { return DestDir; }
-  bool insertSeen(StringRef Filename) { return Seen.insert(Filename).second; }
-  void addFile(StringRef Filename, StringRef FileDst = {});
+  virtual bool insertSeen(StringRef Filename) { return 
Seen.insert(Filename).second; }
+  virtual void addFile(StringRef Filename, StringRef FileDst = {});
 
-  void addFileMapping(StringRef VPath, StringRef RPath) {
+  virtual void addFileMapping(StringRef VPath, StringRef RPath) {
 VFSWriter.addFileMapping(VPath, RPath);
   }
 
-  void attachToPreprocessor(Preprocessor &PP) override;
-  void attachToASTReader(ASTReader &R) override;
+  virtual void attachToPreprocessor(Preprocessor &PP) override;
+  virtual void attachToASTReader(ASTReader &R) override;
 
-  void writeFileMap();
-  bool hasErrors() { return HasErrors; }
+  virtual void writeFileMap();
+  virtual bool hasErrors() { return HasErrors; }
 };
 
 /// AttachDependencyGraphGen - Create a dependency graph generator, and attach


Index: clang/include/clang/Frontend/Utils.h
===
--- clang/include/clang/Frontend/Utils.h
+++ clang/include/clang/Frontend/Utils.h
@@ -145,18 +145,18 @@
   ~ModuleDependencyCollector() override { writeFileMap(); }
 
   StringRef getDest() { return DestDir; }
-  bool insertSeen(StringRef Filename) { return Seen.insert(Filename).second; }
-  void addFile(StringRef Filename, StringRef FileDst = {});
+  virtual bool insertSeen(StringRef Filename) { return Seen.insert(Filename).second; }
+  virtual void addFile(StringRef Filename, StringRef FileDst = {});
 
-  void addFileMapping(StringRef VPath, StringRef RPath) {
+  virtual void addFileMapping(StringRef VPath, StringRef RPath) {
 VFSWriter.addFileMapping(VPath, RPath);
   }
 
-  void attachToPreprocessor(Preprocessor &PP) override;
-  void attachToASTReader(ASTReader &R) override;
+  virtual void attachToPreprocessor(Preprocessor &PP) override;
+  virtual void attachToASTReader(ASTReader &R) override;
 
-  void writeFileMap();
-  bool hasErrors() { return HasErrors; }
+  virtual void writeFileMap();
+  virtual bool hasErrors() { return HasErrors; }
 };
 
 /// AttachDependencyGraphGen - Create a dependency graph generator, and attach
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D58072#1393640 , @bruno wrote:

> How much of the `ModuleDependencyCollector` will be reused as is by LLDB? I 
> wonder about the tradeoff versus inheriting from `DependencyCollector` 
> directly.


We reuse the `attachTo` methods, which in turn use the 
`ModuleDependencyListener`, `ModuleDependencyPPCallbacks` and 
`ModuleDependencyMMCallbacks`. Do you think it's worth re-implementing those?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58072



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

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D58072#1393817 , @bruno wrote:

> Not really. Would making only the `attachTo*` methods virtual enough though?


You mean making them **not** virtual? They're the only ones I don't override.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58072



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

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Alright, I'll split this up in two patches


Repository:
  rC Clang

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

https://reviews.llvm.org/D58072



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

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353882: Make ModuleDependencyCollector's method virtual 
(NFC) (authored by JDevlieghere, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58072?vs=186320&id=186538#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58072

Files:
  cfe/trunk/include/clang/Frontend/Utils.h


Index: cfe/trunk/include/clang/Frontend/Utils.h
===
--- cfe/trunk/include/clang/Frontend/Utils.h
+++ cfe/trunk/include/clang/Frontend/Utils.h
@@ -145,18 +145,18 @@
   ~ModuleDependencyCollector() override { writeFileMap(); }
 
   StringRef getDest() { return DestDir; }
-  bool insertSeen(StringRef Filename) { return Seen.insert(Filename).second; }
-  void addFile(StringRef Filename, StringRef FileDst = {});
+  virtual bool insertSeen(StringRef Filename) { return 
Seen.insert(Filename).second; }
+  virtual void addFile(StringRef Filename, StringRef FileDst = {});
 
-  void addFileMapping(StringRef VPath, StringRef RPath) {
+  virtual void addFileMapping(StringRef VPath, StringRef RPath) {
 VFSWriter.addFileMapping(VPath, RPath);
   }
 
   void attachToPreprocessor(Preprocessor &PP) override;
   void attachToASTReader(ASTReader &R) override;
 
-  void writeFileMap();
-  bool hasErrors() { return HasErrors; }
+  virtual void writeFileMap();
+  virtual bool hasErrors() { return HasErrors; }
 };
 
 /// AttachDependencyGraphGen - Create a dependency graph generator, and attach


Index: cfe/trunk/include/clang/Frontend/Utils.h
===
--- cfe/trunk/include/clang/Frontend/Utils.h
+++ cfe/trunk/include/clang/Frontend/Utils.h
@@ -145,18 +145,18 @@
   ~ModuleDependencyCollector() override { writeFileMap(); }
 
   StringRef getDest() { return DestDir; }
-  bool insertSeen(StringRef Filename) { return Seen.insert(Filename).second; }
-  void addFile(StringRef Filename, StringRef FileDst = {});
+  virtual bool insertSeen(StringRef Filename) { return Seen.insert(Filename).second; }
+  virtual void addFile(StringRef Filename, StringRef FileDst = {});
 
-  void addFileMapping(StringRef VPath, StringRef RPath) {
+  virtual void addFileMapping(StringRef VPath, StringRef RPath) {
 VFSWriter.addFileMapping(VPath, RPath);
   }
 
   void attachToPreprocessor(Preprocessor &PP) override;
   void attachToASTReader(ASTReader &R) override;
 
-  void writeFileMap();
-  bool hasErrors() { return HasErrors; }
+  virtual void writeFileMap();
+  virtual bool hasErrors() { return HasErrors; }
 };
 
 /// AttachDependencyGraphGen - Create a dependency graph generator, and attach
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37390: [diagtool] Add list-warning-flags

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere created this revision.
JDevlieghere added a project: clang.
Herald added a subscriber: mgorny.

This patch adds a new tool to diagnostic tool called `list-warning-flags` to 
display only warnings that have a corresponding -W flag.

While we already have `list-warnings`, the output contains all internal 
diagnostic names. This is an implementation detail of the compiler and usually 
not what the users wants. I've rephrased the description of the exiting tool to 
emphasize its respective purpose.

Because the limited amount of code and complexity, as well as the absence of 
arguments in the other tools and the existing duplication in features between 
the them, it seemed reasonable to me to add this as something separate. However 
I don't know too much about diagtool and its usage so this is very much open 
for discussion.


Repository:
  rL LLVM

https://reviews.llvm.org/D37390

Files:
  tools/diagtool/CMakeLists.txt
  tools/diagtool/ListWarningFlags.cpp
  tools/diagtool/ListWarnings.cpp

Index: tools/diagtool/ListWarnings.cpp
===
--- tools/diagtool/ListWarnings.cpp
+++ tools/diagtool/ListWarnings.cpp
@@ -1,13 +1,13 @@
-//===- ListWarnings.h - diagtool tool for printing warning flags --===//
+//===- ListWarnings.h - diagtool tool for printing warnings ---===//
 //
 // The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===--===//
 //
-// This file provides a diagtool tool that displays warning flags for
+// This file provides a diagtool tool that displays all warning for
 // diagnostics.
 //
 //===--===//
@@ -21,20 +21,20 @@
 #include "llvm/Support/Format.h"
 
 DEF_DIAGTOOL("list-warnings",
- "List warnings and their corresponding flags",
+ "List all (internal) warnings",
  ListWarnings)
-  
+
 using namespace clang;
 using namespace diagtool;
 
 namespace {
 struct Entry {
   llvm::StringRef DiagName;
   llvm::StringRef Flag;
-  
+
   Entry(llvm::StringRef diagN, llvm::StringRef flag)
 : DiagName(diagN), Flag(flag) {}
-  
+
   bool operator<(const Entry &x) const { return DiagName < x.DiagName; }
 };
 }
@@ -52,57 +52,57 @@
 int ListWarnings::run(unsigned int argc, char **argv, llvm::raw_ostream &out) {
   std::vector Flagged, Unflagged;
   llvm::StringMap > flagHistogram;
-  
+
   ArrayRef AllDiagnostics = getBuiltinDiagnosticsByName();
 
   for (ArrayRef::iterator di = AllDiagnostics.begin(),
 de = AllDiagnostics.end();
di != de; ++di) {
 unsigned diagID = di->DiagID;
-
+
 if (DiagnosticIDs::isBuiltinNote(diagID))
   continue;
-
+
 if (!DiagnosticIDs::isBuiltinWarningOrExtension(diagID))
   continue;
-  
+
 Entry entry(di->getName(),
 DiagnosticIDs::getWarningOptionForDiag(diagID));
-
+
 if (entry.Flag.empty())
   Unflagged.push_back(entry);
 else {
   Flagged.push_back(entry);
   flagHistogram[entry.Flag].push_back(diagID);
 }
   }
-  
+
   out << "Warnings with flags (" << Flagged.size() << "):\n";
   printEntries(Flagged, out);
-  
+
   out << "Warnings without flags (" << Unflagged.size() << "):\n";
   printEntries(Unflagged, out);
 
   out << "\nSTATISTICS:\n\n";
 
-  double percentFlagged = ((double) Flagged.size()) 
+  double percentFlagged = ((double) Flagged.size())
 / (Flagged.size() + Unflagged.size()) * 100.0;
-  
-  out << "  Percentage of warnings with flags: " 
+
+  out << "  Percentage of warnings with flags: "
   << llvm::format("%.4g",percentFlagged) << "%\n";
-  
+
   out << "  Number of unique flags: "
   << flagHistogram.size() << '\n';
-  
+
   double avgDiagsPerFlag = (double) Flagged.size() / flagHistogram.size();
   out << "  Average number of diagnostics per flag: "
   << llvm::format("%.4g", avgDiagsPerFlag) << '\n';
 
   out << "  Number in -Wpedantic (not covered by other -W flags): "
   << flagHistogram["pedantic"].size() << '\n';
 
   out << '\n';
-  
+
   return 0;
 }
 
Index: tools/diagtool/ListWarningFlags.cpp
===
--- /dev/null
+++ tools/diagtool/ListWarningFlags.cpp
@@ -0,0 +1,55 @@
+//===- ListWarningFlags.h - diagtool tool for printing warning flags --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This file provides a diagtool tool that displays only warnings that have a
+// corresponding flags for diagnostics.
+//
+//===-

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 113731.
JDevlieghere added a comment.
Herald added subscribers: aheejin, klimek.

Thanks for the review Adrian!

I somehow completely overlooked the existing `--flags-only` option in `diagtool 
tree`. Obviously it makes much more sense to implement printing flags here and 
make it the default behavior. I've also added some code to print the flags with 
the appropriate color, i.e. defaults are printed in green while the others 
remain printed in yellow as before.

I've updated the diff to reflect this and added a test case.


Repository:
  rL LLVM

https://reviews.llvm.org/D37390

Files:
  test/Tooling/diatool.test
  tools/diagtool/TreeView.cpp

Index: tools/diagtool/TreeView.cpp
===
--- tools/diagtool/TreeView.cpp
+++ tools/diagtool/TreeView.cpp
@@ -32,10 +32,10 @@
 public:
   llvm::raw_ostream &out;
   const bool ShowColors;
-  bool FlagsOnly;
+  bool Internal;
 
   TreePrinter(llvm::raw_ostream &out)
-  : out(out), ShowColors(hasColors(out)), FlagsOnly(false) {}
+  : out(out), ShowColors(hasColors(out)), Internal(false) {}
 
   void setColor(llvm::raw_ostream::Colors Color) {
 if (ShowColors)
@@ -54,23 +54,41 @@
 return Diags.isIgnored(DiagID, SourceLocation());
   }
 
+  static bool enabledByDefault(const GroupRecord &Group) {
+for (auto I = Group.diagnostics_begin(), E = Group.diagnostics_end();
+ I != E; ++I) {
+  if (isIgnored(I->DiagID))
+return false;
+}
+
+for (auto I = Group.subgroup_begin(), E = Group.subgroup_end(); I != E;
+ ++I) {
+  if (!enabledByDefault(*I))
+return false;
+}
+
+return true;
+  }
+
   void printGroup(const GroupRecord &Group, unsigned Indent = 0) {
 out.indent(Indent * 2);
 
-setColor(llvm::raw_ostream::YELLOW);
+if (enabledByDefault(Group))
+  setColor(llvm::raw_ostream::GREEN);
+else
+  setColor(llvm::raw_ostream::YELLOW);
+
 out << "-W" << Group.getName() << "\n";
 resetColor();
 
 ++Indent;
-for (GroupRecord::subgroup_iterator I = Group.subgroup_begin(),
-E = Group.subgroup_end();
- I != E; ++I) {
+for (auto I = Group.subgroup_begin(), E = Group.subgroup_end(); I != E;
+ ++I) {
   printGroup(*I, Indent);
 }
 
-if (!FlagsOnly) {
-  for (GroupRecord::diagnostics_iterator I = Group.diagnostics_begin(),
- E = Group.diagnostics_end();
+if (Internal) {
+  for (auto I = Group.diagnostics_begin(), E = Group.diagnostics_end();
I != E; ++I) {
 if (ShowColors && !isIgnored(I->DiagID))
   setColor(llvm::raw_ostream::GREEN);
@@ -107,12 +125,9 @@
 ArrayRef AllGroups = getDiagnosticGroups();
 llvm::DenseSet NonRootGroupIDs;
 
-for (ArrayRef::iterator I = AllGroups.begin(),
- E = AllGroups.end();
- I != E; ++I) {
-  for (GroupRecord::subgroup_iterator SI = I->subgroup_begin(),
-  SE = I->subgroup_end();
-   SI != SE; ++SI) {
+for (auto I = AllGroups.begin(), E = AllGroups.end(); I != E; ++I) {
+  for (auto SI = I->subgroup_begin(), SE = I->subgroup_end(); SI != SE;
+   ++SI) {
 NonRootGroupIDs.insert((unsigned)SI.getID());
   }
 }
@@ -139,16 +154,16 @@
 };
 
 static void printUsage() {
-  llvm::errs() << "Usage: diagtool tree [--flags-only] []\n";
+  llvm::errs() << "Usage: diagtool tree [--internal] []\n";
 }
 
 int TreeView::run(unsigned int argc, char **argv, llvm::raw_ostream &out) {
   // First check our one flag (--flags-only).
-  bool FlagsOnly = false;
+  bool Internal = false;
   if (argc > 0) {
 StringRef FirstArg(*argv);
-if (FirstArg.equals("--flags-only")) {
-  FlagsOnly = true;
+if (FirstArg.equals("--internal")) {
+  Internal = true;
   --argc;
   ++argv;
 }
@@ -175,7 +190,7 @@
   }
 
   TreePrinter TP(out);
-  TP.FlagsOnly = FlagsOnly;
+  TP.Internal = Internal;
   TP.showKey();
   return ShowAll ? TP.showAll() : TP.showGroup(RootGroup);
 }
Index: test/Tooling/diatool.test
===
--- /dev/null
+++ test/Tooling/diatool.test
@@ -0,0 +1,816 @@
+RUN: diagtool tree | FileCheck %s
+
+CHECK: -W
+CHECK:   -Wextra
+CHECK: -Wmissing-field-initializers
+CHECK: -Wignored-qualifiers
+CHECK: -Winitializer-overrides
+CHECK: -Wsemicolon-before-method-body
+CHECK: -Wmissing-method-return-type
+CHECK: -Wsign-compare
+CHECK: -Wunused-parameter
+CHECK: -W#pragma-messages
+CHECK: -WCFString-literal
+CHECK: -WCL4
+CHECK:   -Wall
+CHECK: -Wmost
+CHECK:   -Wchar-subscripts
+CHECK:   -Wcomment
+CHECK:   -Wdelete-non-virtual-dtor
+CHECK:   -Wfor-loop-analysis
+CHECK:   -Wformat
+CHECK: -Wformat-extra-args
+CHECK: -Wformat-ze

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 113737.
JDevlieghere added a comment.

Thanks Alex!


https://reviews.llvm.org/D37390

Files:
  test/Misc/warning-flags-tree.c
  tools/diagtool/TreeView.cpp

Index: tools/diagtool/TreeView.cpp
===
--- tools/diagtool/TreeView.cpp
+++ tools/diagtool/TreeView.cpp
@@ -32,10 +32,10 @@
 public:
   llvm::raw_ostream &out;
   const bool ShowColors;
-  bool FlagsOnly;
+  bool Internal;
 
   TreePrinter(llvm::raw_ostream &out)
-  : out(out), ShowColors(hasColors(out)), FlagsOnly(false) {}
+  : out(out), ShowColors(hasColors(out)), Internal(false) {}
 
   void setColor(llvm::raw_ostream::Colors Color) {
 if (ShowColors)
@@ -54,23 +54,41 @@
 return Diags.isIgnored(DiagID, SourceLocation());
   }
 
+  static bool enabledByDefault(const GroupRecord &Group) {
+for (auto I = Group.diagnostics_begin(), E = Group.diagnostics_end();
+ I != E; ++I) {
+  if (isIgnored(I->DiagID))
+return false;
+}
+
+for (auto I = Group.subgroup_begin(), E = Group.subgroup_end(); I != E;
+ ++I) {
+  if (!enabledByDefault(*I))
+return false;
+}
+
+return true;
+  }
+
   void printGroup(const GroupRecord &Group, unsigned Indent = 0) {
 out.indent(Indent * 2);
 
-setColor(llvm::raw_ostream::YELLOW);
+if (enabledByDefault(Group))
+  setColor(llvm::raw_ostream::GREEN);
+else
+  setColor(llvm::raw_ostream::YELLOW);
+
 out << "-W" << Group.getName() << "\n";
 resetColor();
 
 ++Indent;
-for (GroupRecord::subgroup_iterator I = Group.subgroup_begin(),
-E = Group.subgroup_end();
- I != E; ++I) {
+for (auto I = Group.subgroup_begin(), E = Group.subgroup_end(); I != E;
+ ++I) {
   printGroup(*I, Indent);
 }
 
-if (!FlagsOnly) {
-  for (GroupRecord::diagnostics_iterator I = Group.diagnostics_begin(),
- E = Group.diagnostics_end();
+if (Internal) {
+  for (auto I = Group.diagnostics_begin(), E = Group.diagnostics_end();
I != E; ++I) {
 if (ShowColors && !isIgnored(I->DiagID))
   setColor(llvm::raw_ostream::GREEN);
@@ -107,12 +125,9 @@
 ArrayRef AllGroups = getDiagnosticGroups();
 llvm::DenseSet NonRootGroupIDs;
 
-for (ArrayRef::iterator I = AllGroups.begin(),
- E = AllGroups.end();
- I != E; ++I) {
-  for (GroupRecord::subgroup_iterator SI = I->subgroup_begin(),
-  SE = I->subgroup_end();
-   SI != SE; ++SI) {
+for (auto I = AllGroups.begin(), E = AllGroups.end(); I != E; ++I) {
+  for (auto SI = I->subgroup_begin(), SE = I->subgroup_end(); SI != SE;
+   ++SI) {
 NonRootGroupIDs.insert((unsigned)SI.getID());
   }
 }
@@ -139,16 +154,16 @@
 };
 
 static void printUsage() {
-  llvm::errs() << "Usage: diagtool tree [--flags-only] []\n";
+  llvm::errs() << "Usage: diagtool tree [--internal] []\n";
 }
 
 int TreeView::run(unsigned int argc, char **argv, llvm::raw_ostream &out) {
   // First check our one flag (--flags-only).
-  bool FlagsOnly = false;
+  bool Internal = false;
   if (argc > 0) {
 StringRef FirstArg(*argv);
-if (FirstArg.equals("--flags-only")) {
-  FlagsOnly = true;
+if (FirstArg.equals("--internal")) {
+  Internal = true;
   --argc;
   ++argv;
 }
@@ -175,7 +190,7 @@
   }
 
   TreePrinter TP(out);
-  TP.FlagsOnly = FlagsOnly;
+  TP.Internal = Internal;
   TP.showKey();
   return ShowAll ? TP.showAll() : TP.showGroup(RootGroup);
 }
Index: test/Misc/warning-flags-tree.c
===
--- test/Misc/warning-flags-tree.c
+++ test/Misc/warning-flags-tree.c
@@ -1,6 +1,6 @@
-// RUN: diagtool tree | FileCheck -strict-whitespace %s
-// RUN: diagtool tree -Weverything | FileCheck -strict-whitespace %s
-// RUN: diagtool tree everything | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal -Weverything | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal everything | FileCheck -strict-whitespace %s
 //
 // These three ways of running diagtool tree are the same:
 // they produce a tree for every top-level diagnostic flag.
@@ -30,7 +30,7 @@
 // RUN: not diagtool tree -Wthis-is-not-a-valid-flag
 
 
-// RUN: diagtool tree -Wgnu | FileCheck -strict-whitespace -check-prefix CHECK-GNU %s
+// RUN: diagtool tree --internal -Wgnu | FileCheck -strict-whitespace -check-prefix CHECK-GNU %s
 // CHECK-GNU: -Wgnu
 // CHECK-GNU:   -Wgnu-designator
 // CHECK-GNU: ext_gnu_array_range
@@ -40,7 +40,7 @@
 // CHECK-GNU: ext_vla
 // There are more GNU extensions but we don't need to check them all.
 
-// RUN: diagtool tree --flags-only -Wgnu | FileCheck -check-prefix CHECK-FLAGS-ON

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL312546: [diagtool] Change default tree behavior to print 
only flags (authored by JDevlieghere).

Changed prior to commit:
  https://reviews.llvm.org/D37390?vs=113737&id=113885#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37390

Files:
  cfe/trunk/test/Misc/warning-flags-tree.c
  cfe/trunk/tools/diagtool/TreeView.cpp

Index: cfe/trunk/tools/diagtool/TreeView.cpp
===
--- cfe/trunk/tools/diagtool/TreeView.cpp
+++ cfe/trunk/tools/diagtool/TreeView.cpp
@@ -32,10 +32,10 @@
 public:
   llvm::raw_ostream &out;
   const bool ShowColors;
-  bool FlagsOnly;
+  bool Internal;
 
   TreePrinter(llvm::raw_ostream &out)
-  : out(out), ShowColors(hasColors(out)), FlagsOnly(false) {}
+  : out(out), ShowColors(hasColors(out)), Internal(false) {}
 
   void setColor(llvm::raw_ostream::Colors Color) {
 if (ShowColors)
@@ -54,19 +54,37 @@
 return Diags.isIgnored(DiagID, SourceLocation());
   }
 
+  static bool enabledByDefault(const GroupRecord &Group) {
+for (const DiagnosticRecord &DR : Group.diagnostics()) {
+  if (isIgnored(DR.DiagID))
+return false;
+}
+
+for (const GroupRecord &GR : Group.subgroups()) {
+  if (!enabledByDefault(GR))
+return false;
+}
+
+return true;
+  }
+
   void printGroup(const GroupRecord &Group, unsigned Indent = 0) {
 out.indent(Indent * 2);
 
-setColor(llvm::raw_ostream::YELLOW);
+if (enabledByDefault(Group))
+  setColor(llvm::raw_ostream::GREEN);
+else
+  setColor(llvm::raw_ostream::YELLOW);
+
 out << "-W" << Group.getName() << "\n";
 resetColor();
 
 ++Indent;
 for (const GroupRecord &GR : Group.subgroups()) {
   printGroup(GR, Indent);
 }
 
-if (!FlagsOnly) {
+if (Internal) {
   for (const DiagnosticRecord &DR : Group.diagnostics()) {
 if (ShowColors && !isIgnored(DR.DiagID))
   setColor(llvm::raw_ostream::GREEN);
@@ -132,16 +150,16 @@
 };
 
 static void printUsage() {
-  llvm::errs() << "Usage: diagtool tree [--flags-only] []\n";
+  llvm::errs() << "Usage: diagtool tree [--internal] []\n";
 }
 
 int TreeView::run(unsigned int argc, char **argv, llvm::raw_ostream &out) {
   // First check our one flag (--flags-only).
-  bool FlagsOnly = false;
+  bool Internal = false;
   if (argc > 0) {
 StringRef FirstArg(*argv);
-if (FirstArg.equals("--flags-only")) {
-  FlagsOnly = true;
+if (FirstArg.equals("--internal")) {
+  Internal = true;
   --argc;
   ++argv;
 }
@@ -168,7 +186,7 @@
   }
 
   TreePrinter TP(out);
-  TP.FlagsOnly = FlagsOnly;
+  TP.Internal = Internal;
   TP.showKey();
   return ShowAll ? TP.showAll() : TP.showGroup(RootGroup);
 }
Index: cfe/trunk/test/Misc/warning-flags-tree.c
===
--- cfe/trunk/test/Misc/warning-flags-tree.c
+++ cfe/trunk/test/Misc/warning-flags-tree.c
@@ -1,6 +1,6 @@
-// RUN: diagtool tree | FileCheck -strict-whitespace %s
-// RUN: diagtool tree -Weverything | FileCheck -strict-whitespace %s
-// RUN: diagtool tree everything | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal -Weverything | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal everything | FileCheck -strict-whitespace %s
 //
 // These three ways of running diagtool tree are the same:
 // they produce a tree for every top-level diagnostic flag.
@@ -29,8 +29,7 @@
 
 // RUN: not diagtool tree -Wthis-is-not-a-valid-flag
 
-
-// RUN: diagtool tree -Wgnu | FileCheck -strict-whitespace -check-prefix CHECK-GNU %s
+// RUN: diagtool tree --internal -Wgnu | FileCheck -strict-whitespace -check-prefix CHECK-GNU %s
 // CHECK-GNU: -Wgnu
 // CHECK-GNU:   -Wgnu-designator
 // CHECK-GNU: ext_gnu_array_range
@@ -40,7 +39,7 @@
 // CHECK-GNU: ext_vla
 // There are more GNU extensions but we don't need to check them all.
 
-// RUN: diagtool tree --flags-only -Wgnu | FileCheck -check-prefix CHECK-FLAGS-ONLY %s
+// RUN: diagtool tree -Wgnu | FileCheck -check-prefix CHECK-FLAGS-ONLY %s
 // CHECK-FLAGS-ONLY: -Wgnu
 // CHECK-FLAGS-ONLY:   -Wgnu-designator
 // CHECK-FLAGS-ONLY-NOT: ext_gnu_array_range
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62654: [Docs] Modernize references to macOS

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62654



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

[PATCH] D65116: [Driver] Set the default win32-macho debug format to DWARF

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/Driver/ToolChains/MSVC.h:81
 
   /// Set CodeView as the default debug info format. Users can use -gcodeview
   /// and -gdwarf to override the default.

Let's update the comment. 


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

https://reviews.llvm.org/D65116



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

[PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/include/clang/Basic/FileManager.h:217
   ///
-  /// This returns NULL if the file doesn't exist.
+  /// This returns a \c std::error_code if there was an error loading the file.
   ///

harlanhaskins wrote:
> jkorous wrote:
> > Does that mean that it's now safe to assume the value is `!= NULL` in the 
> > absence of errors?
> That's the intent of these changes, yes, but it should also be documented. 👍
In line with the previous comment, should we just pass a reference then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



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

[PATCH] D65854: [diagtool] Use `operator<<(Colors)` to print out colored output.

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM with Fangrui's comment addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65854



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

[PATCH] D65545: Handle some fs::remove failures

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Thanks for looping me in, @vsapsai!




Comment at: llvm/include/llvm/Support/FileUtilities.h:52-53
   if (DeleteIt) {
-// Ignore problems deleting the file.
-sys::fs::remove(Filename);
+if (std::error_code EC = sys::fs::remove(Filename))
+  report_fatal_error("failed removing file \"" + Filename + "\": " + 
EC.message());
   }

vsapsai wrote:
> For this change opinion of LLDB developers can be useful as it changes 
> existing `FileRemover` behavior.
Aborting isn't acceptable for LLDB. Can we turn this into an assert instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65545



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

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:172
+  case VFSMode::Unknown:
+if (!this->VFS) {
+  LLVM_FALLTHROUGH;

Is this really necessary? 



Comment at: clang/lib/Driver/Driver.cpp:980
 
+  // We might try accessing the VFS fairly eatly, so make sure we have the
+  // right one.

`s/eatly/early/`



Comment at: clang/test/Driver/vfsmode.py:18
+current = os.path.realpath(os.curdir)
+name_max = int(subprocess.check_output("getconf NAME_MAX /", shell=True))
+path_max = int(subprocess.check_output("getconf PATH_MAX /", shell=True))

Can we pass `current` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65986



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

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Two small suggestions for the test, but this change LGTM.




Comment at: clang/test/Driver/vfsmode.py:33
+new_name_len = target_len - len(absolute) - 1
+new_name_len = new_name_len if new_name_len < name_max else name_max
+absolute = os.path.join(absolute, 'x' * new_name_len)

I like the Pythonic approach, but I wonder if `new_name_len = min(target_len - 
len(absolute) - 1, name_max)` wouldn't be easier to read.  



Comment at: clang/test/Driver/vfsmode.py:48
+# exceed PATH_MAX.
+assert os.path.exists(file_relative_path)
+

Should we add an `assert not os.path.exists(absolute)` for good measure?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65986



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

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:172
+  case VFSMode::Unknown:
+if (!this->VFS) {
+  LLVM_FALLTHROUGH;

jfb wrote:
> JDevlieghere wrote:
> > Is this really necessary? 
> the `Driver` ctor takes a `VFS` parameter which some tools set (otherwise it 
> defaults to `nullptr`), so if the command-line `VFS` isn't specified we want 
> to leave whatever was passed in as-is. If the command line parameter is set 
> the we'll obey that instead.
> 
> So yes I think it's the right thing to do.
FWIW, I got confused by the indentation of the case element. Even though it's 
in the if-case it just acts like a label.  While clever, I think for the sake 
of readability it might be worth just repeating `this->VFS = 
llvm::vfs::createPhysicalFileSystem().release();`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65986



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

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Thanks for bearing with me, JF!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65986



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

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/include/clang/Driver/Options.td:2012
+  HelpText<"Use the virtual file system in 'real' mode, or 'physical' mode. In 
'real' mode the working directory is linked to the process' working directory. 
In 'physical' mode it has its own working directory, independent of (but 
initially equal to) that of the process.">,
+  Values<"real,physical">;
 def imultilib : Separate<["-"], "imultilib">, Group;

jfb wrote:
> sammccall wrote:
> > Mea culpa: naming these `physical`/`real` doesn't really explain either the 
> > implementation difference nor the observable effect.
> > (I thought this internal name didn't matter much and didn't want to rename 
> > `getRealFilesystem`...)
> > 
> > Can we avoid propagating these bad names further? Maybe instead something 
> > like `-os-workdir` or `-workdir=os|virtual` or similar?
> Maybe? What do other folks who actually tough VFS think? I'm happy to go 
> either way, I was following what seemed to be the prevalent naming (and 
> parroted the comments on those functions) but I understand that it might not 
> have been naming that you thought would leave those function 🙂
Personally I like the `-workdir=os|virtual` because  it better conveys the 
semantic meaning, rather than how it's implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65986



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

[PATCH] D66240: Remove LVALUE / RVALUE workarounds

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66240



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

[PATCH] D64526: [NFC] Unforget a colon in a few CHECK: directives.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

The dsymutil part LGTM


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64526



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

[PATCH] D67613: [DWARF-5] Support for DWARF-5 C++ language tags

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I reverted this in r372672 because it caused a test failure in LLDB: 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/1748/


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67613



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

[PATCH] D45255: [CodeGen] Add an option to suppress output of llvm.ident

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: test/CodeGen/no-ident-version.c:1
+// RUN: %clang_cc1 -Qn -emit-llvm -debug-info-kind=limited -o - %s | FileCheck 
%s
+

Please test both -Qy and -Qn as well as the default case. 


https://reviews.llvm.org/D45255



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

[PATCH] D45045: [DebugInfo] Generate debug information for labels.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Please run your changes through clang format before checking this in; otherwise 
LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D45045



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

[PATCH] D45255: [CodeGen] Add an option to suppress output of llvm.ident

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I’m happy with the test, thanks!

In https://reviews.llvm.org/D45255#1072335, @aprantl wrote:

> I'm not sure. Compatibility with GCC and getting identical output for 
> debugging purposes seem to be at odds here. I personally lean slightly 
> towards stripping it from the debug info as well.


If it were the other way around I’d agree with you, but given that -Qn becomes 
the default, I think we should not strip it from the debug info.


https://reviews.llvm.org/D45255



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

[PATCH] D45255: [CodeGen] Add an option to suppress output of llvm.ident

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D45255#1073710, @miyuki wrote:

> In https://reviews.llvm.org/D45255#1073703, @JDevlieghere wrote:
>
> > If it were the other way around I’d agree with you, but given that -Qn 
> > becomes the default, I think we should not strip it from the debug info.
>
>
> No, the default is -Qy


Ah, right, I was confused by the CHECK-NONE prefix, I would have  merged it 
with the default case. In that case I agree with Adrian. :-)


Repository:
  rC Clang

https://reviews.llvm.org/D45255



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

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I tested this patch on the LLDB test suite and all tests passed.

What I did was:

- I removed the DWARF version check in clang so this was always generated.
- I commented out the code that reads the .apple_objc accelerator tables in 
DWARFASTParserClang.cpp (which as far as I can tell is the only consumer). I 
expected to have to add logic to read the methods but the code already takes 
care of that, and just had a special if-clause for Objective-C classes. This is 
actually quite nice, as we don't need a code change to make both things working 
next to each other.


https://reviews.llvm.org/D48241



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

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 152883.
JDevlieghere added a comment.

- Update diff to version I used for testing (modulo the removed DWARF version 
check)


https://reviews.llvm.org/D48241

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,15 @@
   /// Cache of previously constructed interfaces which may change.
   llvm::SmallVector ObjCInterfaceCache;
 
+  struct ObjCMethodCacheEntry {
+const ObjCMethodDecl *MD;
+llvm::DISubprogram *DIMethodDecl;
+  };
+
+  /// Cache of forward declarations for methods belonging to the interface.
+  llvm::DenseMap>
+  ObjCMethodCache;
+
   /// Cache of references to clang modules and precompiled headers.
   llvm::DenseMap ModuleCache;
 
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3346,6 +3346,27 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+  const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+  QualType QTy(ID->getTypeForDecl(), 0);
+  auto it = TypeCache.find(QTy.getAsOpaquePtr());
+  if (it != TypeCache.end()) {
+llvm::DICompositeType *InterfaceDecl =
+cast(it->second);
+llvm::DISubprogram *FD = DBuilder.createFunction(
+InterfaceDecl, Name, LinkageName, Unit, LineNo,
+getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+TParamsArray.get());
+DBuilder.finalizeSubprogram(FD);
+ObjCMethodCache[ID].push_back({OMD, FD});
+  }
+}
+  }
+
   // Push the function onto the lexical block stack.
   LexicalBlockStack.emplace_back(SP);
 
@@ -4213,6 +4234,30 @@
 DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
   }
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto p : ObjCMethodCache) {
+  if (p.second.empty())
+continue;
+
+  QualType QTy(p.first->getTypeForDecl(), 0);
+  auto it = TypeCache.find(QTy.getAsOpaquePtr());
+  if (it == TypeCache.end())
+continue;
+
+  llvm::DICompositeType *InterfaceDecl =
+  cast(it->second);
+
+  SmallVector EltTys;
+  auto CurrenetElts = InterfaceDecl->getElements();
+  EltTys.append(CurrenetElts.begin(), CurrenetElts.end());
+  for (auto &M : p.second)
+EltTys.push_back(M.DIMethodDecl);
+  llvm::DINodeArray Elements = DBuilder.getOrCreateArray(EltTys);
+  DBuilder.replaceArrays(InterfaceDecl, Elements);
+}
+  }
+
   for (auto p : ReplaceMap) {
 assert(p.second);
 auto *Ty = cast(p.second);


Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,15 @@
   /// Cache of previously constructed interfaces which may change.
   llvm::SmallVector ObjCInterfaceCache;
 
+  struct ObjCMethodCacheEntry {
+const ObjCMethodDecl *MD;
+llvm::DISubprogram *DIMethodDecl;
+  };
+
+  /// Cache of forward declarations for methods belonging to the interface.
+  llvm::DenseMap>
+  ObjCMethodCache;
+
   /// Cache of references to clang modules and precompiled headers.
   llvm::DenseMap ModuleCache;
 
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3346,6 +3346,27 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+  const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+  QualType QTy(ID->getTypeForDecl(), 0);
+  auto it = TypeCache.find(QTy.getAsOpaquePtr());
+  if (it != TypeCache.end()) {
+llvm::DICompositeType *InterfaceDecl =
+cast(it->second);
+llvm::DISubprogram *FD = DBuilder.createFunction(
+InterfaceDecl, Name, LinkageName, Unit, LineNo,
+getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+TParamsArray.get());
+DBuilder.finalizeSubprogram(FD);
+ObjCMethodCache[ID].push_back({OMD, FD});
+  }
+}
+  }
+
   // Push the function onto the lexical block stack.
   LexicalBlock

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 152896.
JDevlieghere added a comment.

- Re-add test case (forgot to stage it for last patch)


https://reviews.llvm.org/D48241

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGenObjC/debug-info-category.m

Index: test/CodeGenObjC/debug-info-category.m
===
--- /dev/null
+++ test/CodeGenObjC/debug-info-category.m
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+// RUN: %clang_cc1 -dwarf-version=4 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF4
+
+@interface Foo {
+  int integer;
+}
+
+- (int)integer;
+- (id)integer:(int)_integer;
+@end
+
+@implementation Foo
+- (int)integer {
+  return integer;
+}
+
+- (id)integer:(int)_integer {
+  integer = _integer;
+  return self;
+}
+@end
+
+@interface Foo (Bar)
+- (id)add:(Foo *)addend;
+@end
+
+@implementation Foo (Bar)
+- (id)add:(Foo *)addend {
+  return [self integer:[self integer] + [addend integer]];
+}
+@end
+
+// 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) 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) add:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: true
Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,15 @@
   /// Cache of previously constructed interfaces which may change.
   llvm::SmallVector ObjCInterfaceCache;
 
+  struct ObjCMethodCacheEntry {
+const ObjCMethodDecl *MD;
+llvm::DISubprogram *DIMethodDecl;
+  };
+
+  /// Cache of forward declarations for methods belonging to the interface.
+  llvm::DenseMap>
+  ObjCMethodCache;
+
   /// Cache of references to clang modules and precompiled headers.
   llvm::DenseMap ModuleCache;
 
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3346,6 +3346,27 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+  const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+  QualType QTy(ID->getTypeForDecl(), 0);
+  auto it = TypeCache.find(QTy.getAsOpaquePtr());
+  if (it != TypeCache.end()) {
+llvm::DICompositeType *InterfaceDecl =
+cast(it->second);
+llvm::DISubprogram *FD = DBuilder.createFunction(
+InterfaceDecl, Name, LinkageName, Unit, LineNo,
+getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+TParamsArray.get());
+DBuilder.finalizeSubprogram(FD);
+ObjCMethodCache[ID].push_back({OMD, FD});
+  }
+}
+  }
+
   // Push the function onto the lexical block stack.
   LexicalBlockStack.emplace_back(SP);
 
@@ -4213,6 +4234,30 @@
 DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
   }
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto p : ObjCMethodCache) {
+  if (p.second.empty())
+continue;
+
+  QualType QTy(p.first->getTypeForDecl(), 0);
+  auto it = TypeCache.find(QTy.getAsOpaquePtr());
+  if (it == TypeCache.end())
+continue;
+
+  llvm::DICompositeType *InterfaceDecl =
+  cast(it->second);
+
+  SmallVector EltTys;
+  auto CurrenetElts = InterfaceDecl->getElements();
+  EltTys.append(CurrenetElts.begin(), CurrenetElts.end());
+  for (auto &M : p.second)
+EltTys.push_back(M.DIMethodDecl);
+  llvm::DINodeArray Elements = DBuilder.getOrCreateArray(EltTys);
+  DBuilder.replaceArrays(InterfaceDecl, Elements);
+}
+  }
+
   for (auto p : ReplaceMap) {
 assert(p.second);
 auto 

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 152910.
JDevlieghere marked 6 inline comments as done.
JDevlieghere added a comment.

- Feedback Adrian & Duncan


https://reviews.llvm.org/D48241

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGenObjC/debug-info-category.m

Index: test/CodeGenObjC/debug-info-category.m
===
--- /dev/null
+++ test/CodeGenObjC/debug-info-category.m
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+// RUN: %clang_cc1 -dwarf-version=4 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF4
+
+@interface Foo {
+  int integer;
+}
+
+- (int)integer;
+- (id)integer:(int)_integer;
+@end
+
+@implementation Foo
+- (int)integer {
+  return integer;
+}
+
+- (id)integer:(int)_integer {
+  integer = _integer;
+  return self;
+}
+@end
+
+@interface Foo (Bar)
++ (id)zero:(Foo *)zeroend;
+- (id)add:(Foo *)addend;
+@end
+
+@implementation Foo (Bar)
++ (id)zero:(Foo *)zeroend {
+  return [self integer:0];
+}
+- (id)add:(Foo *)addend {
+  return [self integer:[self integer] + [addend integer]];
+}
+@end
+
+// 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: false
+// DWARF4-NOT: !DISubprogram(name: "-[Foo(Bar) add:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "+[Foo(Bar) zero:]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: true
Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,10 @@
   /// Cache of previously constructed interfaces which may change.
   llvm::SmallVector ObjCInterfaceCache;
 
+  /// Cache of forward declarations for methods belonging to the interface.
+  llvm::DenseMap>
+  ObjCMethodCache;
+
   /// Cache of references to clang modules and precompiled headers.
   llvm::DenseMap ModuleCache;
 
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3346,6 +3346,27 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+  const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+  QualType QTy(ID->getTypeForDecl(), 0);
+  auto It = TypeCache.find(QTy.getAsOpaquePtr());
+  if (It != TypeCache.end()) {
+llvm::DICompositeType *InterfaceDecl =
+cast(It->second);
+llvm::DISubprogram *FD = DBuilder.createFunction(
+InterfaceDecl, Name, LinkageName, Unit, LineNo,
+getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+TParamsArray.get());
+DBuilder.finalizeSubprogram(FD);
+ObjCMethodCache[ID].push_back(FD);
+  }
+}
+  }
+
   // Push the function onto the lexical block stack.
   LexicalBlockStack.emplace_back(SP);
 
@@ -4213,6 +4234,29 @@
 DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
   }
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto P : ObjCMethodCache) {
+  if (P.second.empty())
+continue;
+
+  QualType QTy(P.first->getTypeForDecl(), 0);
+  auto It = TypeCache.find(QTy.getAsOpaquePtr());
+  assert(It != TypeCache.end());
+
+  llvm::DICompositeType *InterfaceDecl =
+  cast(It->second);
+
+  SmallVector EltTys;
+  auto CurrenetElts = InterfaceDecl->getElements();
+  EltTys.append(CurrenetElts.begin(), CurrenetElts.end());
+  for (auto &

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4239
+// Add methods to interface.
+for (auto p : ObjCMethodCache) {
+  if (p.second.empty())

dexonsmith wrote:
> Some comment for "p" here.
Fixed; I'll update the rest of the file in a follow-up to keep things 
consistent.



Comment at: lib/CodeGen/CGDebugInfo.cpp:4246
+  if (it == TypeCache.end())
+continue;
+

aprantl wrote:
> What does it mean that a type is not being found in the cache here?
Replaced it with an assert. 


https://reviews.llvm.org/D48241



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

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC335757: [DebugInfo] Emit ObjC methods as part of interface 
(authored by JDevlieghere, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D48241

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  test/CodeGenObjC/debug-info-category.m

Index: test/CodeGenObjC/debug-info-category.m
===
--- test/CodeGenObjC/debug-info-category.m
+++ test/CodeGenObjC/debug-info-category.m
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+// RUN: %clang_cc1 -dwarf-version=4 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF4
+
+@interface Foo {
+  int integer;
+}
+
+- (int)integer;
+- (id)integer:(int)_integer;
+@end
+
+@implementation Foo
+- (int)integer {
+  return integer;
+}
+
+- (id)integer:(int)_integer {
+  integer = _integer;
+  return self;
+}
+@end
+
+@interface Foo (Bar)
++ (id)zero:(Foo *)zeroend;
+- (id)add:(Foo *)addend;
+@end
+
+@implementation Foo (Bar)
++ (id)zero:(Foo *)zeroend {
+  return [self integer:0];
+}
+- (id)add:(Foo *)addend {
+  return [self integer:[self integer] + [addend integer]];
+}
+@end
+
+// 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: false
+// DWARF4-NOT: !DISubprogram(name: "-[Foo(Bar) add:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "+[Foo(Bar) zero:]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: true
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3346,6 +3346,27 @@
   if (HasDecl && isa(D))
 DeclCache[D->getCanonicalDecl()].reset(SP);
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+  const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+  QualType QTy(ID->getTypeForDecl(), 0);
+  auto It = TypeCache.find(QTy.getAsOpaquePtr());
+  if (It != TypeCache.end()) {
+llvm::DICompositeType *InterfaceDecl =
+cast(It->second);
+llvm::DISubprogram *FD = DBuilder.createFunction(
+InterfaceDecl, Name, LinkageName, Unit, LineNo,
+getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+TParamsArray.get());
+DBuilder.finalizeSubprogram(FD);
+ObjCMethodCache[ID].push_back(FD);
+  }
+}
+  }
+
   // Push the function onto the lexical block stack.
   LexicalBlockStack.emplace_back(SP);
 
@@ -4213,6 +4234,29 @@
 DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
   }
 
+  if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto P : ObjCMethodCache) {
+  if (P.second.empty())
+continue;
+
+  QualType QTy(P.first->getTypeForDecl(), 0);
+  auto It = TypeCache.find(QTy.getAsOpaquePtr());
+  assert(It != TypeCache.end());
+
+  llvm::DICompositeType *InterfaceDecl =
+  cast(It->second);
+
+  SmallVector EltTys;
+  auto CurrenetElts = InterfaceDecl->getElements();
+  EltTys.append(CurrenetElts.begin(), CurrenetElts.end());
+  for (auto &MD : P.second)
+EltTys.push_back(MD);
+  llvm::DINodeArray Elements = DBuilder.getOrCreateArray(EltTys);
+  DBuilder.replaceArrays(InterfaceDecl, Elements);
+}
+  }
+
   for (auto p : ReplaceMap) {
 assert(p.second);
 auto *Ty = cast(p.second);
Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGD

[PATCH] D69213: Avoid appending the source directory to an absolute path

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM modulo the comment.


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

https://reviews.llvm.org/D69213



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

[PATCH] D69213: Avoid appending the source directory to an absolute path

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:541
+MainFileDir = MainFile->getDir()->getName();
+if (MainFileDir != "." && !llvm::sys::path::is_absolute(MainFileName)) {
   llvm::SmallString<1024> MainFileDirSS(MainFileDir);

Should we check if `MainFileDir` is absolute to, instead of just checking for 
`.`?


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

https://reviews.llvm.org/D69213



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

[PATCH] D80770: [diagtool] Install diagtool when LLVM_INSTALL_TOOLCHAIN_ONLY is ON.

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Thanks, Volodymyr!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80770



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

[PATCH] D84565: [Darwin] [Driver] Clang should invoke dsymutil for lto builds -g*

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.

When you don’t pass any specific options to the linker, it’s going to generate 
a temporary `lto.o` file in `/tmp` and delete it after the link. When 
`dsymutil` will try to read that, it won't be there anymore. Unless I'm missing 
I don't think this is going to work. If it turns out I'm mistaken please add 
that situation as a test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84565



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

[PATCH] D84572: Allow .dSYM's to be directly placed in an alternate directory

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/include/clang/Driver/Options.td:693
 def e : JoinedOrSeparate<["-"], "e">, Group;
+def external_dsym_dir : JoinedOrSeparate<["-"], "external-dsym-dir">,
+  Flags<[DriverOption, RenderAsInput]>,

Could we drop the `external` prefix or did you pick this for consistency with 
other flags? It seems rather specific for the externalize debug info scenario 
while the functionality seems generic enough by itself. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84572



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

[PATCH] D84572: Allow .dSYM's to be directly placed in an alternate directory

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84572

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

[PATCH] D83454: [CMake] Make `intrinsics_gen` dependency unconditional.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

LGTM. I verified this works for the build-tree standalone build of LLDB, but 
please still keep an eye on 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-standalone/ after you 
land this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83454



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

[PATCH] D83454: [CMake] Make `intrinsics_gen` dependency unconditional.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D83454#2142719 , @michele.scandale 
wrote:

> I tested locally the standalone build of Clang with D83426 
> .
>  I just want to make sure that we all agree this is right way moving forward.


Yep, with the target exported this is the right thing to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83454



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

[PATCH] D82733: [clang][docs] Add note about using `-flto` with `-g` on macOS

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/docs/CommandGuide/clang.rst:477
 
+  Note: on Darwin, when using :option:`-flto` along with :option:`-g` and
+  compiling and linking in separate steps, you also need to pass

Any reason not to use the sphinx note directive (`.. note::` )?

https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#paragraph-level-markup


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82733



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

[PATCH] D82733: [clang][docs] Add note about using `-flto` with `-g` on macOS

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM. Thank you for adding this!

In D82733#2121414 , @phil-blain wrote:

> I tested the sphinx build for the `man` and `html` targets. Any other targets 
> I should check ?


No, that sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82733



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