[PATCH] D29432: Modular Codegen: Emit inline asm into users, not modular objects

2017-02-01 Thread David Blaikie via Phabricator via cfe-commits
dblaikie created this revision. The best semantics I can come up with (based on discussions with Richard) for inline asm and namespace scope internal linkage variables in modular headers is to emit them into all users and never into modular objects. The reason for this is that such asm might

[PATCH] D29432: Modular Codegen: Emit inline asm into users, not modular objects

2017-02-01 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Oh, one of the reasons I was sort of compelled to keep the 'classic' AST file compilation working was that when looking at the change history of the test cases exercising it I found that some changes had been motivated by a filed LLVM bug:

[PATCH] D29205: Change debug-info-for-profiling from a TargetOption to a function attribute.

2017-02-01 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Oops, forgot to write stuff so the approval appears on-list. Anyway - looks good/approved. https://reviews.llvm.org/D29205 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D28845: Prototype of modules codegen

2017-01-24 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 85599. dblaikie added a comment. - Move linkage handling into GetGVALinkageForFunction https://reviews.llvm.org/D28845 Files: include/clang/AST/ASTContext.h include/clang/AST/ExternalASTSource.h include/clang/Basic/LangOptions.def

[PATCH] D28845: Prototype of modules codegen

2017-01-23 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 85484. dblaikie added a comment. Add bit to the Module record for when modular codegen decls are included in the MODULAR_CODEGEN_DECLS bitcode record https://reviews.llvm.org/D28845 Files: include/clang/AST/ASTContext.h

[PATCH] D29233: Fix linkage of static locals in available_externally functions to be DiscardableODR/linkonce_odr

2017-01-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 86137. dblaikie added a comment. Improve comment/mention Itanium ABI https://reviews.llvm.org/D29233 Files: lib/AST/ASTContext.cpp test/CodeGenCXX/explicit-instantiation.cpp Index: test/CodeGenCXX/explicit-instantiation.cpp

[PATCH] D29233: Fix linkage of static locals in available_externally functions to be DiscardableODR/linkonce_odr

2017-01-27 Thread David Blaikie via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL293344: Fix linkage of static locals in available_externally functions to be… (authored by dblaikie). Changed prior to commit: https://reviews.llvm.org/D29233?vs=86137=86138#toc Repository: rL LLVM

[PATCH] D28845: Prototype of modules codegen

2017-01-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 86140. dblaikie added a comment. - More test coverage (for local static variables) https://reviews.llvm.org/D28845 Files: include/clang/AST/ASTContext.h include/clang/AST/ExternalASTSource.h include/clang/Basic/LangOptions.def

[PATCH] D29233: Fix linkage of static locals in available_externally functions to be DiscardableODR/linkonce_odr

2017-01-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie created this revision. As Mehdi put it, entities should either be available_externally+weak_odr, or linkonce_odr+linkonce_odr. While some functions are emitted a_e/weak, their local variables were emitted a_e/linkonce_odr. While it might be nice to emit them a_e/weak, the Itanium ABI (&

[PATCH] D28845: Prototype of modules codegen

2017-01-29 Thread David Blaikie via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. dblaikie marked 2 inline comments as done. Closed by commit rL293456: Prototype of modules codegen (authored by dblaikie). Changed prior to commit: https://reviews.llvm.org/D28845?vs=86200=86239#toc Repository: rL LLVM

[PATCH] D28845: Prototype of modules codegen

2017-01-24 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 85686. dblaikie added a comment. - Add test coverage https://reviews.llvm.org/D28845 Files: include/clang/AST/ASTContext.h include/clang/AST/ExternalASTSource.h include/clang/Basic/LangOptions.def include/clang/Basic/Module.h

[PATCH] D28845: Prototype of modules codegen

2017-01-24 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 85649. dblaikie added a comment. - Add cc1 flag for modular codegen, -fmodule-codegen https://reviews.llvm.org/D28845 Files: include/clang/AST/ASTContext.h include/clang/AST/ExternalASTSource.h include/clang/Basic/LangOptions.def

[PATCH] D28845: Prototype of modules codegen

2017-01-28 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 86200. dblaikie added a comment. - Address code review feedback - Formatting https://reviews.llvm.org/D28845 Files: include/clang/AST/ASTContext.h include/clang/AST/ExternalASTSource.h include/clang/Basic/LangOptions.def

[PATCH] D28845: Prototype of modules codegen

2017-01-28 Thread David Blaikie via Phabricator via cfe-commits
dblaikie marked 6 inline comments as done. dblaikie added a comment. Addressed CR feedback Comment at: lib/AST/ExternalASTSource.cpp:33 +ExternalASTSource::hasExternalDefinitions(unsigned ID) { + return EK_ReplyHazy; +} rsmith wrote: > You should add support

[PATCH] D30220: Only enable AddDiscriminator pass when -fdebug-info-for-profiling is true

2017-02-21 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Did you want to enable all DebugInfoForProfiling features when there's a sample profile specified? (not sure if all of the features are needed when stitching things back up, or if some of

[PATCH] D29901: Modular Codegen: Add/use a bit in serialized function definitions to track whether they are the subject of modular codegen

2017-02-16 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 88792. dblaikie added a comment. - Simplify ModuleFile lookup https://reviews.llvm.org/D29901 Files: include/clang/AST/ExternalASTSource.h include/clang/Sema/MultiplexExternalSemaSource.h include/clang/Serialization/ASTReader.h

[PATCH] D29901: Modular Codegen: Add/use a bit in serialized function definitions to track whether they are the subject of modular codegen

2017-02-13 Thread David Blaikie via Phabricator via cfe-commits
dblaikie created this revision. Some decls are created not where they are written, but in other module files/users (implicit special members and function template implicit specializations). To correctly identify them, use a bit next to the definition to track the modular codegen property.

[PATCH] D29432: Modular Codegen: Emit inline asm into users, not modular objects

2017-02-13 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. +rsmith we chatted about a bit of this offline & some things came out of it, not entirely resolved: 1. Is this the right thing for inline asm or similar things (static functions? static variables?) - I was mirroring the choice already made for static variables

[PATCH] D28845: Prototype of modules codegen

2017-01-17 Thread David Blaikie via Phabricator via cfe-commits
dblaikie created this revision. First pass at generating weak definitions of inline functions from module files (& currently skipping definitions of those functions in uses) I've done some manual testing (haven't delved into the preferred testing strategy for modules). Seems to work for simplest

[PATCH] D30378: [DebugInfo] [DWARFv5] Collect calling convention info for C++ types during codegen

2017-02-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Be best to avoid having to 'fixup' the argabi after the fact - and instead build the type with the right argabi from the get-go. Comment at: lib/CodeGen/CGDebugInfo.cpp:955-957 +// DWARFv5 doesn't specify explicit DW_CC_* value for this case, +

[PATCH] D29901: Modular Codegen: Add/use a bit in serialized function definitions to track whether they are the subject of modular codegen

2017-02-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 89931. dblaikie added a comment. - Simplify ModuleFile lookup - Build ModularCodegenDecls list from the same place the modular codegen bit is set on the decl - Cleanup no-longer-needed changes to DeclMustBeEmitted/isRequiredDecl

[PATCH] D28162: [ADT] Delete RefCountedBaseVPTR.

2016-12-29 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks for the cleanup! Comment at: llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp:14-37 +struct VirtualRefCounted : public llvm::RefCountedBase { +

[PATCH] D31153: Add the ability to use the children() range API in a const-correct manner

2017-03-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. As I mentioned to Craig Topper recently on another review, generally when implementing const and non-const overloads the non-const is implemented in terms of the const overload (& const_casts away const on the result). This ensures no UB if the const overload is

[PATCH] D31440: PR32382: Adapt to LLVM changes in DIExpression.

2017-03-29 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. I'm a bit confused - the alloca was only emitted at -O0, by the looks of it. Presumably it's pessimizing in some way at higher optimization levels? Or is that not the case? Also, it looks like this change lost the "if > gmlt" test, so might cause variable

[PATCH] D31153: Add the ability to use the children() range API in a const-correct manner

2017-04-11 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me - thanks! Comment at: include/clang/AST/StmtIterator.h:137 + inline friend StmtIterator + cast_away_const(const struct ConstStmtIterator ); };

[PATCH] D31153: Add the ability to use the children() range API in a const-correct manner

2017-04-11 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added inline comments. Comment at: include/clang/AST/Expr.h:4025 child_range children() { +const_child_range CCR = const_cast(this)->children(); +return child_range(cast_away_const(CCR.begin()), If this is adding const, can you use a weaker

[PATCH] D29901: Modular Codegen: Add/use a bit in serialized function definitions to track whether they are the subject of modular codegen

2017-04-11 Thread David Blaikie via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL299982: Modular Codegen: Add/use a bit in serialized function definitions to track… (authored by dblaikie). Changed prior to commit: https://reviews.llvm.org/D29901?vs=91193=94886#toc Repository: rL

[PATCH] D31440: PR32382: Adapt to LLVM changes in DIExpression.

2017-04-03 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. In https://reviews.llvm.org/D31440#713396, @aprantl wrote: > In https://reviews.llvm.org/D31440#713308, @dblaikie wrote: > > > I'm a bit confused - the alloca was only emitted at -O0, by the looks of > > it. Presumably it's pessimizing in some way at higher

[PATCH] D29901: Modular Codegen: Add/use a bit in serialized function definitions to track whether they are the subject of modular codegen

2017-03-09 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Ping I've got a relatively small patch to add support for debug info types after this one & working on a patch on top of that to enable one/the other/both of these (to test their value independently/together). Might be more expedient to sit down & push through them

[PATCH] D29901: Modular Codegen: Add/use a bit in serialized function definitions to track whether they are the subject of modular codegen

2017-03-09 Thread David Blaikie via Phabricator via cfe-commits
dblaikie updated this revision to Diff 91193. dblaikie added a comment. sync/resolve https://reviews.llvm.org/D29901 Files: include/clang/AST/ASTContext.h include/clang/AST/ExternalASTSource.h include/clang/Sema/MultiplexExternalSemaSource.h include/clang/Serialization/ASTReader.h

[PATCH] D34891: Replace assert(0) with llvm_unreachable.

2017-07-10 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. If possible, please upload patches with full context - makes it easier to check around nearby code when reviewing. (the 'arc' tool for phab helps with this) Comment at: include/clang/AST/Redeclarable.h:257-261 if (PassedFirst) { -

[PATCH] D35564: Suppress -pedantic warnings about GNU extension StmtExpr in glibc's assert macro

2017-07-18 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Ping - Richard, what'd you reckon? (This came up on another review where -Wzero-as-null-pointer triggered in a system header in libstdc++ I think) https://reviews.llvm.org/D35564 ___ cfe-commits mailing list

[PATCH] D35559: [CMake][Modules] Tweak Modules-unfriendly builds

2017-07-18 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. I know Richard Smith has a modules buildbot - so I'm curious: How is it working without these changes? Are these changes an effort to broaden the coverage of modular self hosting builds (what's the current coverage? (only LLVM proper) what's the new coverage? (all

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

2017-07-21 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added inline comments. Comment at: include/clang/AST/TemplateBase.h:109-111 +// This is the type that most closely resembles what is in the source. +// At the moment it is retaining typedefs, but not decltype or typeof. +uintptr_t DisplayType;

[PATCH] D34860: [Objective-C] Fix non-determinism in clang

2017-07-05 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds plausible to me https://reviews.llvm.org/D34860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D36474: Use the file name from linemarker for debug info if an input is preprocessed source.

2017-08-08 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Looks plausible (not sure how nice it is to use the llvm::Module itself to communicate the file name to this bit of logic - I'll leave that to Richard Smith to judge, I think) Comment at: test/CodeGen/debug-info-preprocessed-file.i:10 +// RUN:

[PATCH] D33437: Emit available_externally vtables opportunistically

2017-05-29 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:1383-1385 + if (!OpportunisticVTables.empty()) +assert(shouldOpportunisticallyEmitVTables() && + "Only emit opportunistic vtables with optimizations"); Perhaps this:

[PATCH] D33711: [TableGen] Clang changes to support Record::getValueAsString and getValueAsListOfStrings returning StringRef instead of std::string

2017-05-31 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Few questions, but address them as you see fit. Comment at: utils/TableGen/ClangDiagnosticsEmitter.cpp:1280-1281 writeHeader((IsRemarkGroup ? "-R" : "-W") + -

[PATCH] D33705: [CGVTables] Finalize SP before attempting to clone it

2017-06-01 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. I guess this would need a cross-project test case (ie: it'd have to run LLVM optimizations to fail/pass/demonstrate the fix). I think it'd be OK to add one if there's a neat/clean/obvious

[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-15 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks fine - thanks https://reviews.llvm.org/D34052 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-13 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. I take it there's already handling for these attributes on non-member functions? There's probably a reason this code can't be wherever that code is & subtract one from the index? (reducing code duplication by having all the error handling, etc, in one place/once)

[PATCH] D33102: [clang] Implement -Wcast-qual for C++

2017-06-05 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. I still feel like that's more testing than would be ideal (does the context of the cast matter? Wether it's dereferenced, a struct member access, assigned, initialized, etc - it doesn't

[PATCH] D33941: [Driver] Add test to cover case when LSan is not supported

2017-06-06 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Repository: rL LLVM https://reviews.llvm.org/D33941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37428: Debug info: Fixed faulty debug locations for attributed statements

2017-09-05 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Not sure what you mean by "avoid emitting unnecessary stop points" - do you have a test case for that? https://reviews.llvm.org/D37428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37428: Debug info: Fixed faulty debug locations for attributed statements

2017-09-05 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. OK, sounds good - thanks :) https://reviews.llvm.org/D37428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

2017-09-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks OK to me - couple of minor questions. Comment at: include/clang/Frontend/CodeGenOptions.def:222 ///< of inline stack frames

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

2017-09-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. > I would prefer to eliminate the `` from the instance name as well, > because our debugger reconstructs a name more to its liking from the > parameter children. However, IIUC the name with params is used for > deduplication in LTO, so that is probably not such a

[PATCH] D36386: [clang] Remove unit test which uses reverse-iterate flag

2017-08-24 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. sounds good - so long as other tests would fail if the fix this test was testing wasn't present (on a reverse iteration enabled build) https://reviews.llvm.org/D36386

[PATCH] D37235: Let -Wdelete-non-virtual-dtor fire in system headers too.

2017-08-28 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. (if you'd prefer to wait for Reid or Richard to take a look, that's OK too :) ) Looks good to me, though wouldn't mind if the tests were a bit simpler - if they can be made so.

[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file

2017-09-04 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Couple of drive by comments - no doubt Richard will have a more in depth review, but figured this might save a round or two. Comment at: lib/Frontend/CompilerInvocation.cpp:1561-1565 + if (File.empty()) + { +

[PATCH] D40601: [XRay][clang] Introduce -fxray-always-emit-customevents

2017-11-29 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Not totally clear to me why this feature is desirable, but assume you've got use cases/reasons :) Comment at: clang/lib/Driver/XRayArgs.cpp:32-33 constexpr char

[PATCH] D41357: WIP: Fix Diagnostic layering, moving diagnostics out of Basic

2017-12-18 Thread David Blaikie via Phabricator via cfe-commits
dblaikie created this revision. dblaikie added a reviewer: rsmith. Herald added subscribers: cfe-commits, klimek. This goes part-way down the path of moving the actual diagnostics out of Clang's Basic library into the respective libraries that use those diagnostics. The end goal would be that a

[PATCH] D41387: Remove llvm::MemoryBuffer const_casts

2017-12-19 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems good Repository: rC Clang https://reviews.llvm.org/D41387 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39428: [Analyzer] As suggested, use value storage for BodyFarm

2017-10-30 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! https://reviews.llvm.org/D39428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46998: [XRay][clang+compiler-rt] Make XRay depend on a C++ standard lib

2018-05-17 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. "In the future we can revisit this when we have a better idea as to why not depending on the C++ ABI functionality is a better solution." - this was discussed previously in the thread linked from the bug. A big thing, so far as I understand it, is that Clang doesn't

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

2018-06-15 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. 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

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

2018-06-04 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a subscriber: Higuoxing. dblaikie added a comment. Probably CC someone from apple here & ask about rdar://8678458 - they can look it up & provide the missing context. https://reviews.llvm.org/D47687 ___ cfe-commits mailing list

[PATCH] D47953: [builtin] Add bitfield support for __builtin_dump_struct

2018-06-29 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a subscriber: paulsemel. dblaikie added a comment. Yeah, doesn't look like this code handles a value crossing the boundary of the size of the bitfield type (it's reading only the low bit). I suspect looking at the code that generates bitfield accesses would be useful - and/or

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

2018-06-26 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:4239 +// Add methods to interface. +for (auto p : ObjCMethodCache) { + if (p.second.empty()) aprantl wrote: > JDevlieghere wrote: > > dexonsmith wrote: > > > Some comment for

[PATCH] D47953: [builtin] Add bitfield support for __builtin_dump_struct

2018-06-25 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. This doesn't seem to build for me - so hard to debug/probe it: llvm/src/tools/clang/lib/CodeGen/CGBuiltin.cpp:1264:65: error: no viable conversion from 'clang::QualType' to 'llvm::Type *' CGF.CGM.getDataLayout().getTypeSizeInBits(CanonicalType),

[PATCH] D48571: improve diagnostics for missing 'template' keyword

2018-06-26 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks pretty good to me - nice work! Repository: rL LLVM https://reviews.llvm.org/D48571 ___ cfe-commits mailing list

[PATCH] D42736: [DebugInfo] Improvements to representation of enumeration types (PR36168)

2018-02-02 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. Few things that could tidy up the tests a bit, but otherwise looks good :) Comment at: test/CodeGen/debug-info-enum.cpp:6-10 +// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E0" +// CHECK-SAME: flags:

[PATCH] D42736: [DebugInfo] Improvements to representation of enumeration types (PR36168)

2018-02-01 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added inline comments. Comment at: test/CodeGen/debug-info-enum.cpp:2 +// RUN: %clang -target x86_64-linux -g -S -emit-llvm -o - %s | FileCheck %s +enum class E0 : signed char { + A0 = -128, Could you summarize the purpose of each of these tests

[PATCH] D42736: [DebugInfo] Improvements to representation of enumeration types (PR36168)

2018-01-31 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:2490-2491 for (const auto *Enum : ED->enumerators()) { -Enumerators.push_back(DBuilder.createEnumerator( -Enum->getName(), Enum->getInitVal().getSExtValue())); +const auto =

[PATCH] D42813: [Debug] Annotate compiler generated range-for loop variables.

2018-02-05 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Seems plausible - maybe walking the scopes to count the range-for loops would produce a better number, but would be slower. :/ Dunno. Comment at: lib/Sema/SemaStmt.cpp:2346 +// Assume the variables are nested in the inner scope (loop body). +

[PATCH] D42813: [Debug] Annotate compiler generated range-for loop variables.

2018-02-14 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. In https://reviews.llvm.org/D42813#1007865, @mattd wrote: > Thanks @dblaikie! I renamed the test, and cleaned up per your suggestion. I > originally regex'd the debug-info lines so that the test would verify that > the names were artificial; however, being that we

[PATCH] D42813: [Debug] Annotate compiler generated range-for loop variables.

2018-02-14 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems good, thanks :) Comment at: test/CodeGenCXX/debug-for-range-scope-hints.cpp:1 +// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited %s -o - | FileCheck %s +

[PATCH] D47953: [builtin] Add bitfield support for __builtin_dump_struct

2018-06-21 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Yeah, I know nothing about this dump feature or what's being fixed here - test cases would be great to help motivate/explain. Repository: rC Clang https://reviews.llvm.org/D47953 ___ cfe-commits mailing list

[PATCH] D48426: [clang-cl] Don't emit dllexport inline functions etc. from pch files (PR37801)

2018-06-21 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. In https://reviews.llvm.org/D48426#1139823, @rnk wrote: > `LangOpts.ModulesCodegen` is very related in spirit to this, but I think we > need a distinct option because that was designed to handle all inline > functions (too much), not just dllexport inline functions. +

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

2018-08-02 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Hi - sorry about the stalled opaque pointer types effort. For my money - ideally - if someone comes across a bug caused by incorrect pointee types, ideally that would be fixed by adjusting whatever piece of code was depending on that pointee type being correct to not

[PATCH] D49916: [CodeGen] Add to emitted DebugLoc information about coverage when it's required

2018-07-30 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Test coverage? Repository: rC Clang https://reviews.llvm.org/D49916 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50154: [clangd] capitalize diagnostic messages

2018-08-07 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added subscribers: ilya-biryukov, dblaikie. dblaikie added a comment. What's the motivation for clangd to differ from clang here? (& if the first letter is going to be capitalized, should there be a period at the end? But also the phrasing of most/all diagnostic text isn't in the form of

[PATCH] D50945: [Lex] Make HeaderMaps a unique_ptr vector

2018-08-20 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Repository: rC Clang https://reviews.llvm.org/D50945 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D50945: [Lex] Make HeaderMaps a unique_ptr vector

2018-08-20 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. In https://reviews.llvm.org/D50945#1205337, @kristina wrote: > Given the context (class an file name itself) and documentation around the > function, I don't think in this particular case it improves readability or > maintainability, the lifetime of the `HeaderMap` is

[PATCH] D51177: [DEBUGINFO] Add support for emission of the debug directives only.

2018-08-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added inline comments. Comment at: test/Driver/debug-options.c:259 // +// This tests asserts that "-glineinfo-only" "-g0" disables debug info. +// GLIO_NO: "-cc1" 'lineinfo' seems like two words - the inconsistency with 'line-tables' seems like it'd

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

2018-07-16 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In theory you'd need separate tests for op== and op!= returning false (currently all the tests would pass if both implementations returned true in all cases), but not the biggest deal.

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

2018-07-16 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. Looks good, Thanks! Repository: rC Clang https://reviews.llvm.org/D49265 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

2018-07-16 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Any chance this can/should be unit tested? (also, in general (though might not matter in this instance), symmetric operators like == should be implemented as non-members (though they can still be friends and if they are, can be defined inline in the class definition as a

[PATCH] D51177: [DEBUGINFO] Add support for emission of the debug directives only.

2018-08-30 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Not sure that every test for line-tables-only needs to also test line-directives-only, but not a huge deal either way. (only the cases where there's actually a different codepath to

[PATCH] D51554: [CUDA][OPENMP][NVPTX]Improve logic of the debug info support.

2018-09-04 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. In https://reviews.llvm.org/D51554#1224049, @echristo wrote: > The change in name here from "line tables" to "directives only" feels a bit > confusing. "Limited" seems to be a bit more clear, or even remaining line > tables only. Can you explain where you were going

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

2018-09-04 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me & the answers to others various questions seem well addressed. Repository: rC Clang https://reviews.llvm.org/D51576 ___

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

2018-09-06 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Just to clarify - the general philosophy (there are many cases of exceptions due to complications in various areas) is that clang changes should have clang tests (source code to IR), LLVM changes should have LLVM tests (IR to assembly or object code+dwarf dump) & if

[PATCH] D41357: WIP: Fix Diagnostic layering, moving diagnostics out of Basic

2018-03-14 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Updated with a functional implementation - a few points: - Some tests could use only the Common diagnostics (see one of the intermediate changes where I moved those tests over to use all diagnostics) except for the tablegen required for the diagnostic groups makes

[PATCH] D43779: [Tooling] [0/1] Refactor FrontendActionFactory::create() to return std::unique_ptr<>

2018-04-09 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added subscribers: lebedev.ri, dblaikie. dblaikie added a comment. FWIW - I had some thoughts on this a while back: https://reviews.llvm.org/D4313 Repository: rL LLVM https://reviews.llvm.org/D43779 ___ cfe-commits mailing list

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

2018-04-16 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a subscriber: lebedev.ri. dblaikie added a comment. I'm not sure this is a practical direction to pursue - though perhaps others disagree. It's likely non-trivial to plumb a flag through most build systems to be applied only to test code (& likely would suppress the warning in

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

2018-04-23 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision. dblaikie added inline comments. This revision is now accepted and ready to land. Comment at: lib/Sema/SemaExpr.cpp:11527-11528 - S.Diag(OpLoc, diag::warn_self_assignment) - << LHSDeclRef->getType() - << LHSExpr->getSourceRange() <<

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

2018-04-23 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a subscriber: lebedev.ri. dblaikie added a comment. FWIW I don't fundamentalyl object to also having something like -wtest. Probably needs a better name though (unfortunately the double-negative gets confusing... - like you want to describe the set of diagnostics that should not be

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

2018-04-23 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Is there anything else in the "-w" namespace other than the literal "-w" so far? I mean, I could imagine it might make more sense to default these warnings off & users can turn them on for non-test code, potentially? So "-Wnon-test" might make sense. Repository: rL

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

2018-04-23 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added inline comments. Comment at: lib/Sema/SemaExpr.cpp:11527-11528 - S.Diag(OpLoc, diag::warn_self_assignment) - << LHSDeclRef->getType() - << LHSExpr->getSourceRange() << RHSExpr->getSourceRange(); + S.Diag(OpLoc, IsBuiltin ?

[PATCH] D41357: WIP: Fix Diagnostic layering, moving diagnostics out of Basic

2018-04-19 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. I've pushed this further - it builds now & has no fallback in libBasic to the old behavior/table. One remaining thing I'm unsure about is the table of DIAG_START_* values. Do you think this is OK/good to leave hardcoded in diagnostics? Should I somehow remove the

[PATCH] D44826: Add -Wunused-using, a warning that finds unused using declarations.

2018-03-27 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. You mention a missed case in the description - where the target of a using decl is used and that causes the unused-using not to warn about the using decl? That seems like a big limitation. Does that mean the 'used' bit is being tracked in the wrong place, on the

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a subscriber: lebedev.ri. dblaikie added a comment. Historically Clang's policy on warnings was, I think, much more conservative than it seems to be today. There was a strong desire not to implement off-by-default warnings, and to have warnings with an exceptionally low

[PATCH] D44788: Add an option to support debug fission on implicit ThinLTO.

2018-03-22 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a subscriber: yunlian. dblaikie added a comment. In implicit ThinLTO, the object files are only temporary. Sort of similar to using -gsplit-dwarf when compiling straight to an executable (without using -c): "clang++ x.cpp y.cpp -o a.out" - where should the .dwo files go then? If

[PATCH] D44788: Add an option to support debug fission on implicit ThinLTO.

2018-03-22 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. The only data we have is that where .o files go, .dwo files go beside them. How to generalize this to other situations isn't really obvious to me - even for a.out (do you put all the .dwo files next to a.out? in the same directory? if the names collide, where then? etc).

[PATCH] D44826: Add -Wunused-using, a warning that finds unused using declarations.

2018-03-23 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a subscriber: CarlosAlbertoEnciso. dblaikie added a comment. While implementing the warning is great (wonder if there's any codebase that isn't -Wunused-using clean, that we could use to compare Clang and GCC's behavior broadly - make sure it's catching the same cases (or

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

2018-10-22 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. In https://reviews.llvm.org/D38061#1271021, @twoh wrote: > @aprantl It is a debug info. If you compile > test/CodeGenCXX/debug-info-anonymous.cpp file with `clang -g2 -emit-llvm -S > `, you will find debug metadata something like `distinct !DISubprogram(name: > "foo

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

2018-10-19 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. @echristo > As far as the standard text here, IMO it was just there in case people didn't > have an objcopy around or don't want to split it. I'm not sure why we would > want the ability. I think others have mentioned - but with distributed build it might be easier

[PATCH] D53334: [Frontend] Show diagnostics on prebuilt module configuration mismatch too

2018-10-19 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. Don't know enough about the code to have an opinion on the fix - but in any case this would need a test case, if possible Repository: rC Clang https://reviews.llvm.org/D53334 ___ cfe-commits mailing list

[PATCH] D53329: Generate DIFile with main program if source is not available

2018-10-19 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. @aprantl - yeah, not sure I have any big feels about this (nor do I fully understand it) @yonghong-song - could you explain this maybe in a bit more detail. What behavior does this fix provide? (compared to behavior of existing working cases that don't hit this bug,

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

2018-10-23 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment. In https://reviews.llvm.org/D52296#1272220, @grimar wrote: > Maybe `-gno-dwo`? So we would write `-genable-split-dwarf -gno-dwo`? I'm not sure how everyone else feels about "-g" flags that modify debug behavior (like "-gsplit-dwarf") versus "-f" flags (eg:

  1   2   3   4   5   6   7   >