[PATCH] D50154: [clangd] capitalize diagnostic messages

[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 complete
sentences, so this might not make sense)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50154



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

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

[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 (IMHO) fairly obvious 
> from the const qualifier and from the documentation of the function itself. I 
> would say leave it as is.


I'd vote in favor of the change - since it makes the ownership obvious 
at-a-glance (documentation or not) & helps catch any mistakes that can be 
easily introduced when handling object ownership.


Repository:
  rC Clang

https://reviews.llvm.org/D50945



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

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

[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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 require some 
specific C++ library, so wouldn't this break for users who didn't have the 
specific C++ library installed that compiler-rt for XRay was built against?


https://reviews.llvm.org/D46998



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

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

[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 good idea. :-)

Though you have this out of tree? How do you cope with LTO there?

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




Comment at: test/CodeGenCXX/debug-info-fwd-template-param.cpp:6-17
+template class A {
+public:
+  A(T val);
+private:
+  T x;
+};
+

Probably simpler:

  template class A;
  A *p;

?


https://reviews.llvm.org/D14358



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

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

[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 without .dwo 
files.
+CODEGENOPT(DebugFwdTemplateParams, 1, 0) ///< Whether to emit complete
+ ///< template parameter descriptions 
in

Maybe 'Decl' rather than 'Fwd'.



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

Any particular reason for const int, rather than int?


https://reviews.llvm.org/D14358



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

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

[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 be confusing.

But also I'm not sure line-table versus line-info is very differentiating. 
Maybe line-directives-only?


Repository:
  rC Clang

https://reviews.llvm.org/D51177



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

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

[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 test/new code to validate would I 
bother testing - rather than duplicating all the tests)

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D51177



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

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

[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



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

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

[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 with this particular set of 
> changes in a bit more detail please?


Can't say I have much of an informed opinion about the parts that are only in 
the CUDA code. The "line directives only" terminology did come from a 
suggestion I made in one of the other reviews I can't seem to find right now.. 
ah, here: https://reviews.llvm.org/D51177 - whether or not that matches up with 
the use in the CUDA ToolChain code, I'm not sure.


Repository:
  rC Clang

https://reviews.llvm.org/D51554



___
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

[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 debuginfo-tests are optional/as-desired.


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] D49916: [CodeGen] Add to emitted DebugLoc information about coverage when it's required

[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] D49403: More aggressively complete RecordTypes with Function Pointers

[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 depend on that 
information. Though, yes, there are likely cases where a small bug in the type 
information exposes vast swathes of LLVM code - where the argument might 
reasonably made that the cleanup would take too long to leave things broken. 
Worth seeing what that looks like, though, before making the call - hopefully.


https://reviews.llvm.org/D49403



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

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

[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
+

maybe this test should be called debug-info-range-for-var-names? Just a 
thought. (debug-info seems to be the usual prefix here, and range-for is a 
common shortening of "range-based-for" I think (maybe I'm wrong & other tests 
don't use that naming?)? & 'hints' seems a bit vague as to what the test is 
about - when it's specifically about naming the variables)



Comment at: test/CodeGenCXX/debug-for-range-scope-hints.cpp:28-36
+// CHECK: ![[RANGE1]] = !DILocalVariable(name: "__range1", {{.*}}, flags: 
DIFlagArtificial)
+// CHECK: ![[BEGIN1]] = !DILocalVariable(name: "__begin1", {{.*}}, flags: 
DIFlagArtificial)
+// CHECK: ![[END1]] = !DILocalVariable(name: "__end1",  {{.*}}, flags: 
DIFlagArtificial)
+// CHECK: ![[RANGE2]] = !DILocalVariable(name: "__range2",  {{.*}}, flags: 
DIFlagArtificial)
+// CHECK: ![[BEGIN2]] = !DILocalVariable(name: "__begin2", {{.*}}, flags: 
DIFlagArtificial)
+// CHECK: ![[END2]] = !DILocalVariable(name: "__end2",  {{.*}}, flags: 
DIFlagArtificial)
+// CHECK: ![[RANGE3]] = !DILocalVariable(name: "__range3",  {{.*}}, flags: 
DIFlagArtificial)

I'd drop the ", {{.*}}, flags: DIFlagArtificial)" from these lines - since the 
name's the only bit that's really interesting to this test.


https://reviews.llvm.org/D42813



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

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

[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 already match them as 
> metadata earlier, it's not really that necessary; we are only testing name 
> strings anyways.  Thanks again for the suggestion.


Great - can you commit this yourself or would you like me to do it for you?


https://reviews.llvm.org/D42813



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

[PATCH] D33437: Emit available_externally vtables opportunistically

[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:
  assert(OpportunisticVTables.empty() || shouldOpportunisticallyEmitVTables() 
&& ... )

(it's a bit odd to have a condition that only goes to an assert - rather than 
having both conditions inside the assertion)


https://reviews.llvm.org/D33437



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

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

[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") +
-G->getValueAsString("GroupName"),
+G->getValueAsString("GroupName").str(),
 OS);

Any reason (I'm guessing there is) not to do the str() on the + expression 
instead of the RHS? (seemed like you'd done that in other changes here, to 
minimize the string buffering/reallocation/etc by stringifying once at the top 
level of the expression)



Comment at: utils/TableGen/ClangOptionDocEmitter.cpp:86
 // Pretend no-X and Xno-Y options are aliases of X and XY.
-auto Name = R->getValueAsString("Name");
+std::string Name = R->getValueAsString("Name");
 if (Name.size() >= 4) {

Does this need to be a std::string here? I'm not spotting any mutation (but I 
could well be missing it) of the value. Is it that StringREf doesn't support 
some of these substr - like operations?

Oh, I guess it's that all of these operations want a std::string (for lookup in 
OptionsByName, for the result of string concatenation, etc).

It'd still be marginally more efficient to not have to create a std::string 
up-front, I'd think, but syntactically annoying/verbose for all those uses?



Comment at: utils/TableGen/ClangOptionDocEmitter.cpp:272
+Alias->getValueAsListOfStrings("Prefixes").front(), Alias,
+std::vector(AliasArgs.begin(), AliasArgs.end()), OS);
 OS << ")";

craig.topper wrote:
> Had to make a copy into vector std::string because emitOptionWithArgs has 
> another caller that passes a std::string vector.
Would it be better/OK if the other caller was the one making the copy (it'd be 
cheaper to make a std::vector from a std::vector than 
the other way around - but maybe the other caller is much hotter than this 
one?)?


https://reviews.llvm.org/D33711



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

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

[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 optimization that can be reliably triggered to 
do the cloning that would assert/crash - please add one if you think that's 
practical/reasonable.


https://reviews.llvm.org/D33705



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

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

[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 look like it from the code, etc).

But sure. Could you also (manually, I guess) confirm that this matches GCC's 
cast-qual behavior (insofar as the warning fires in the same situations). If 
there are any deviations, let's chat about them.


Repository:
  rL LLVM

https://reviews.llvm.org/D33102



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

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

[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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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)


https://reviews.llvm.org/D34052



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

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

[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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 client program calling into those libraries would 
be responsible for building the necessary table containing all diagnostics from 
all components they're calling into (so a clang refactoring tool wouldn't need 
to pull in the codegen diagnostics, for example).

But I ran out of momentum a bit & looking for some sanity 
checking/help/encouragement/direction.

The basic idea has been to make the static APIs of DiagnosticIDs non-static, 
then give DiagnosticIDs a ctor that takes the table of diagnostics - as an 
intermediate step, I've left the no-arg ctor in, and dabbled with making it a 
bool ctor just as a way to find the places that still use it and clean them up 
while leaving a handful of correct uses of DiagnosticIDs construction to flesh 
out (build the table from the desired diagnostics, etc) later.

Partly wondering if this is the right general direction, if some of these 
changes are worth committing incrementally as I plumb through DiagnosticIDs 
objects through more APIs.

I remember you, Richard, pointing out that having a generic way to build the 
diagnostics table based on each different clients needs of different subsets of 
all diagnostics wasn't going to be pretty owing to the inability to use the 
preprocessor to conditionally/dynamically #include things in different contexts 
(I'm sure I'm doing a bad job of explaining that - but I'm understanding what 
you meant, that there wouldn't be a quick few lines of "define diagnostics 
table containing these 3 subcomponents" because you'd have to include those 3 
subcomponents diagnostic tables in several different contexts to build up 
several different data structures/subtables/etc... ). & maybe I need to dig 
into that more, look at how that could end up, see if it's reasonable/good 
before doing much more here, rather than putting that off until the end.


Repository:
  rC Clang

https://reviews.llvm.org/D41357

Files:
  include/clang/ARCMigrate/ARCMT.h
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTranslationUnit.h
  include/clang/Frontend/LogDiagnosticPrinter.h
  include/clang/Frontend/SerializedDiagnosticPrinter.h
  include/clang/Frontend/TextDiagnosticPrinter.h
  lib/ARCMigrate/ARCMT.cpp
  lib/ARCMigrate/ARCMTActions.cpp
  lib/ARCMigrate/Internals.h
  lib/ARCMigrate/ObjCMT.cpp
  lib/ARCMigrate/PlistReporter.cpp
  lib/Basic/Diagnostic.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/Basic/Warnings.cpp
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/Driver/Driver.cpp
  lib/Frontend/ChainedIncludesSource.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/LogDiagnosticPrinter.cpp
  lib/Frontend/SerializedDiagnosticPrinter.cpp
  lib/Frontend/TextDiagnosticPrinter.cpp
  lib/Sema/Sema.cpp
  lib/Tooling/CompilationDatabase.cpp
  lib/Tooling/Refactoring.cpp
  lib/Tooling/Tooling.cpp
  tools/arcmt-test/arcmt-test.cpp
  tools/clang-rename/ClangRename.cpp
  tools/diagtool/ListWarnings.cpp
  tools/diagtool/ShowEnabledWarnings.cpp
  tools/driver/cc1_main.cpp
  tools/driver/cc1as_main.cpp
  tools/driver/driver.cpp
  tools/libclang/ARCMigrate.cpp
  tools/libclang/CIndex.cpp
  tools/libclang/CIndexDiagnostic.cpp
  tools/libclang/CXStoredDiagnostic.cpp
  unittests/CrossTU/CrossTranslationUnitTest.cpp
  unittests/Tooling/RewriterTestContext.h

Index: unittests/Tooling/RewriterTestContext.h
===
--- unittests/Tooling/RewriterTestContext.h
+++ unittests/Tooling/RewriterTestContext.h
@@ -38,16 +38,17 @@
: DiagOpts(new DiagnosticOptions()),
  Diagnostics(IntrusiveRefCntPtr(new DiagnosticIDs),
  &*DiagOpts),
- DiagnosticPrinter(llvm::outs(), &*DiagOpts),
+ DiagnosticPrinter(llvm::outs(), Diagnostics.getDiagnosticIDs(),
+   &*DiagOpts),
  InMemoryFileSystem(new vfs::InMemoryFileSystem),
  OverlayFileSystem(
  new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
  Files(FileSystemOptions(), OverlayFileSystem),
  Sources(Diagnostics, Files), Rewrite(Sources, Options) {
-Diagnostics.setClient(&DiagnosticPrinter, false);
-// FIXME: To make these tests truly in-memory, we need to overlay the
-// builtin headers.
-OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+ Diagnostics.setClient(&DiagnosticPrinter, false);
+ // FIXME: To make these tests truly in-memory, we need to overlay the
+ // builtin headers.
+ OverlayFileSystem->pushOverlay(InMemoryFileSystem);
   }
 
   ~RewriterTestContext() {}
Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- unittests/CrossTU/CrossTransl

[PATCH] D41387: Remove llvm::MemoryBuffer const_casts

[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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 that a bit heavier - though 
doing so would help reduce dependencies.
- Naming (of the "DiagnosticTable" header, and the functions/entities contained 
there) could be improved - open to ideas
- API (currently passing in like 6 different raw arrays and ArrayRefs to 
DiagnosticIDs) could be improved - open to ideas


Repository:
  rC Clang

https://reviews.llvm.org/D41357



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

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

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

[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 this change?

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)?


Repository:
  rC Clang

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] D47953: [builtin] Add bitfield support for __builtin_dump_struct

[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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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. + @dblaikie


Does it have to be only the dllexported inline functions - or could this be 
used to home all inline functions? (I guess not - presumably it's not 
acceptable for the compiler to implicitly promote something to dllexport (& if 
it doesn't do that promotion, then the function wouldn't be visible from the 
use))

Is it valid to provide a definition for optimization purposes (LLVM's 
available_externally linkage) for these inline functions? Or is it required 
that, after a change to the header (modifying the implementation of one of 
these dllexported inline functions), the DLL they're exported could be rebuilt 
without rebuilding the clients & the change in behavior would be correctly 
applied?




Comment at: test/CodeGen/pch-dllexport.cpp:22
+// PCH: define weak_odr dso_local dllexport x86_thiscallcc void 
@"?bar@S@@QAEXXZ"
+// PCHWITHOBJ-NOT: define weak_odr dso_local dllexport x86_thiscallcc void 
@"?bar@S@@QAEXXZ"
+};

This is a pretty specific "NOT" - maybe:

  define {{.*}}@"?bar@... 

would be better to ensure no definition is provided? (similarly above)

Also, should this file have some non-exported inline functions to check those 
do the right thing?


https://reviews.llvm.org/D48426



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

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

[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 
to keep it as a single file because your build system already knows how to ship 
around those files.

> That said, if we do I'd rather have it as dwarf5 without split-dwarf as an 
> option rather than a -gsingle-file-split-dwarf option.

Any suggestions on the name?


https://reviews.llvm.org/D52296



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

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

[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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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, for example)


Repository:
  rC Clang

https://reviews.llvm.org/D53329



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

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

[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 /home/twoh/llvms/llvm/tools/clang/test/CodeGenCXX/debug-info-anonymous.cpp:9:3)>",
>  linkageName: "_Z3fooIN1XUt_EEiT_" ...`, which will eventually be included in 
> .debug_info section.


For comparison, GCC names these "foo >" - this is somewhat 
of a compatibility problem, since the template function definition's names 
won't match between GCC and Clang, but I guess this is by far not the only 
instance of that. GCC keeps that naming scheme even when it's clearly ambiguous 
(ie: it's not just making a short name when there's no other X::anonymous enum, 
it does it even when there's two of them, etc)


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

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

[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: 
"-fdebug-types-section"). I kind of personally feel like  "-g" flags are a bit 
odd & would think -f flags to modify debug info behavior, leaving "-g" mostly 
to turn on/off debug info (so, -g, -gN, -gmlt, etc - sort of like -O, -ON, -Os, 
etc).

by that logic -funsplit-split-dwarf ? Mostly goofy suggestion, vaguely trolling 
@echristo to see if he's got other/better ideas, or clearer 
feelings/understanding of history around the flag naming conventions (-f, -g, 
broad naming in general, etc).


https://reviews.llvm.org/D52296



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D38061#1275845, @twoh wrote:

> @dblaikie I see. The problem we're experiencing is that with Clang's naming 
> scheme we end up having different function name between the original source 
> and the preprocessed source (as macro expansion changes the column number).


I imagine that's not achievable, though - as soon as you have a multi-line 
macro at least, it seems like it'd be difficult to preserve (I guess you could 
put loc directives between every line in the macro to ensure every line was 
attributed to the line the macro was written on, maybe?)

> This doesn't work well for me if I want to use distcc on top of our caching 
> system, as the caching scheme expects the output to be same as far as the 
> original source has not been changed (regardless of it's compiled directly or 
> first preprocessed then compiled), but the distcc preprocesses the source 
> locally then sends it to the remote build machine (and we do not turn distcc 
> on for all workflow). I wonder if you have any suggestion to resolve this 
> issue? Thanks!

Best thing to do, I think, if possible, is teach the distributed build system 
not to fully preprocess but to use something like Clang's -frewrite-includes - 
this handles includes, but leaves macro definitions and uses in-place. This 
would preserve Clang's warning semantics (where macros can help inform Clang's 
diagnostic choices, improving diagnostic quality (lowering false positives, 
etc)) and other things like debug info.


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D55394: Re-order type param children of ObjC nodes

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: test/AST/ast-dump-decl.m:90
 // CHECK-NEXT:   -ObjCProtocol {{.+}} 'P'
+// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}}  col:33 T 'id':'id'
 

steveire wrote:
> aaron.ballman wrote:
> > It seems strange to me to print out the type parameter after the superclass 
> > information given the source order. My understanding of the AST dumping 
> > order is that we try to keep the order of nodes in source order whenever 
> > possible.
> That is not really a possible thing to try to do, because the AST dump 
> doesn't relate to a single language. It should be seen as language 
> independent.
> 
> The principle I'm follow is that nodes dump themselves in entirety before 
> starting to dump their child nodes. That is a principle already followed by 
> most nodes. Changing this seems to be low cost, low impact and high benefit 
> to the code.
>That is not really a possible thing to try to do, because the AST dump doesn't 
>relate to a single language. It should be seen as language independent.

Is this particular aspect different between the different source languages 
Clang supports? (could you give examples?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55394



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Thanks for filing the bug/reducing a test case - this change should include an 
automated test case that captures this issue (check for other tests that pass 
-gembed-source to see how this might be tested, possibly expanding the existing 
test case case or introducing a new one (test/CodeGen/debug-info-embed-source.c 
looks like the place to start)

I reduced your test case down a bit further:

test.h:

  int g;

test.c:

  #include "test.h"
  #define MACRO int x;
  MACRO

Though the #include is needed to reproduce the /crash/, but even without the 
#include you can still reproduce the underlying bug - a DIFile witohut source 
and a DIFile with source in the same IR file (one compiled with -gembed-source).

Actually - is that a required invariant, that all DIFiles have source if any of 
them do? That could be a problem for LTO situations that might merge modules 
(one of which may have embedded source and another that doesn't)

Is this fix correct even when the problem occurs in a header file, for instance?

Yeah, seems this problem doesn't occur if you move everything into the header - 
I think it'd be worthwhile to understand/explain why that works out (despite 
the file ID for the macro would be associated with the source range in both 
macro-used-in-header and macro-used-in-primary-source-file cases, so why do 
they need different handling later? Maybe there's a better way to handle this 
more consistently?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D53329



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

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: include/clang/AST/Expr.h:5103
+using reference = AssociationTy;
+using pointer = AssociationTy;
+AssociationIteratorTy() = default;

aaron.ballman wrote:
> riccibruno wrote:
> > aaron.ballman wrote:
> > > Carrying over the conversation from D57098:
> > > 
> > > >> @aaron.ballman Cute, but I suspect this may come back to bite us at 
> > > >> some point. For instance, if someone thinks they're working with a 
> > > >> real pointer, they're likely to expect pointer arithmetic to work when 
> > > >> it won't (at least they'll get compile errors though).
> > > > @riccibruno Hmm, but pointer is just the return type of operator-> no ? 
> > > > Is it actually required to behave like a pointer ? The only requirement 
> > > > I can find is that It->x must be equivalent to (*It).x, which is true 
> > > > here.
> > > 
> > > I double-checked and you're right, this is not a requirement of the STL.
> > > 
> > > > Also looking at the requirements for forward iterators I think that 
> > > > this iterator should actually be downgraded to an input iterator, 
> > > > because of the requirement that reference = T&.
> > > 
> > > My concern is that with the less functional iterators, common algorithms 
> > > get more expensive. For instance, `std::distance()`, `std::advance()` 
> > > become more expensive without random access, and things like 
> > > `std::prev()` become impossible.
> > > 
> > > It seems like the view this needs to provide is a read-only access to the 
> > > `AssociationTy` objects (because we need to construct them on the fly), 
> > > but not the data contained within them. If the only thing you can get 
> > > back from the iterator is a const pointer/reference/value type, then we 
> > > could store a local "current association" object in the iterator and 
> > > return a pointer/reference to that. WDYT?
> > I am worried about lifetime issues with this approach. Returning a 
> > reference/pointer to an `Association` object stored in the iterator means 
> > that the reference/pointer will dangle as soon as the iterator goes out of 
> > scope. This is potentially surprising since the "container" (that is the 
> > `GenericSelectionExpr`) here will still be in scope. Returning a value is 
> > safer in this aspect.
> > 
> > I believe it should be possible to make the iterator a random access 
> > iterator, at least
> > if we are willing to ignore some requirements:
> > 
> > 1.) For forward iterators and up, we must have `reference = T&` or `const 
> > T&`.
> > 2.) For forward iterators and up, `It1 == It2` if and only if `*It1` and 
> > `*It2` are bound to the same object.
> > I am worried about lifetime issues with this approach. Returning a 
> > reference/pointer to an Association object stored in the iterator means 
> > that the reference/pointer will dangle as soon as the iterator goes out of 
> > scope. This is potentially surprising since the "container" (that is the 
> > GenericSelectionExpr) here will still be in scope. Returning a value is 
> > safer in this aspect.
> 
> That's valid.
> 
> > I believe it should be possible to make the iterator a random access 
> > iterator, at least if we are willing to ignore some requirements:
> >
> > 1.) For forward iterators and up, we must have reference = T& or const T&.
> > 2.) For forward iterators and up, It1 == It2 if and only if *It1 and *It2 
> > are bound to the same object.
> 
> Yes, but then passing these iterators to STL algorithms will have UB because 
> we claim to meet the requirements for random access iteration but don't 
> actually meet the requirements. I am not certain what problems might arise 
> from violating these requirements.
> 
> @dblaikie, @mclow.lists: do you have opinions on whether it's okay to not 
> meet these requirements or suggestions on what we could do differently with 
> the design?
My vote is usually "yeah, have a member inside the iterator you return a 
reference/pointer to" to meet the iterator requirements. Yes, it means if you 
keep a pointer/reference to it, that is invalidated when you increment the 
iterator. But that's well established in iterator semantics.

(might be shooting from the hip here/not fully understanding the 
nuances/tradeoffs in this case)



Comment at: include/clang/AST/Expr.h:5084
+  /// storage of Stmt * and TypeSourceInfo * in GenericSelectionExpr.
+  template  class AssociationIteratorTy {
+friend class GenericSelectionExpr;

Worth using any of the iterator helpers LLVM has? (iterator_facade or the like)


Repository:
  rC Clang

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

https://reviews.llvm.org/D57106



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D53329#1373799 , @yonghong-song 
wrote:

> @dblaikie As I mentioned in one of my earlier comments, after some analysis, 
> I think this patch definitely not the right way to fix the problem. We just 
> cannot default to the main file if the DIFile source is not available.
>
> I have a bug filed for the same issue 
> (https://bugs.llvm.org/show_bug.cgi?id=40170) Maybe I should close this patch 
> and put the information on the bug report. Do you agree?


Ah, I understand now - thanks for explaining. Yeah, if you feel like this patch 
isn't the right direction, please mark it "abandoned" & if you're not sure how 
to proceed, probably worth having some discussion on the bug or in an llvm-dev 
thread. (or if you're not sure you have the bandwidth to continue - yeah, 
documenting in the bug anything we've learned so you/someone else can pick it 
up another time would be great)

Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D53329



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

[PATCH] D53940: [Lex] Make MacroDirective::findDirectiveAtLoc take const SourceManager

[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


Repository:
  rC Clang

https://reviews.llvm.org/D53940



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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

> In https://reviews.llvm.org/D52296#1281642, @echristo wrote:
>
> > Can you elaborate on your motivations and what you're trying to do?
> >
> > Thanks!
>
>
> We want to see:
>
> - No extra files. The compiler produces just a .o.
> - The linker leaves most debug info in the .o files.
>
>   That makes the build friendly to existing tools and avoids most of the 
> static linker work.


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

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


https://reviews.llvm.org/D52296



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D52296#1282587, @probinson wrote:

> In https://reviews.llvm.org/D52296#1282458, @dblaikie wrote:
>
> > I guess in that case your distributed build system would have a constraint 
> > that it always ships all the object files back to the primary machine 
> > (where you'd be running the debugger)? (perhaps it just always runs the 
> > link locally - even though it distributes the compilations - I guess that's 
> > probably how things like distcc work, where they only inject themselves 
> > into the compilation command, not the linking command maybe?)
>
>
> AFAIK this is how distcc, icecc, SN-DBS all work.  Compilations are 
> distributed, the .o files come back, links are local.


Thanks! I've never used them, so was just guessing/estimating.

> 
> 
>> Also, may require some work/more flags to handle the possible file renaming 
>> (or relative/absolute adjustment) when object files are built on a remote 
>> system in one location, but then moved back to the local system and placed 
>> in another location?
> 
> Any distributed build has to make this work, so the paths in the line table 
> are usable.  Not clear what you're thinking might be the problem with the 
> split-in-.o case.

Fair point - same sort of problem, but might need distinct handling (ie: might 
not work "for free" with existing tools, but need more support), etc, depending 
on how it's solved?

Mostly just wondering whether Clang would need extra flags to support this - to 
get a sense of the total/overall solution/surface area of the feature, etc.


https://reviews.llvm.org/D52296



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D52296#1282603, @probinson wrote:

> In https://reviews.llvm.org/D52296#1282589, @dblaikie wrote:
>
> > In https://reviews.llvm.org/D52296#1282587, @probinson wrote:
> >
> > > Any distributed build has to make this work, so the paths in the line 
> > > table are usable.  Not clear what you're thinking might be the problem 
> > > with the split-in-.o case.
> >
> >
> > Fair point - same sort of problem, but might need distinct handling (ie: 
> > might not work "for free" with existing tools, but need more support), etc, 
> > depending on how it's solved?
> >
> > Mostly just wondering whether Clang would need extra flags to support this 
> > - to get a sense of the total/overall solution/surface area of the feature, 
> > etc.
>
>
> Shouldn't.  We have licensees using distributed builds all day every day, but 
> we haven't introduced anything into the compiler itself to make that work.  
> On Windows, I know SN-DBS will intercept system calls to patch up filespecs.  
> On Linux I'd expect the remote servers to set up chroot environments so the 
> path names will Just Work, although I admit I've never actually looked.  
> Never had to; it Just Works.


Neat!

> In this patch, where the .o normally points to the .dwo it would instead just 
> point to itself, right?

Right - that's an absolute path, like the line table (between dir+name, if you 
don't override the base dir to something else, etc). So, yeah, if the solutions 
for that are implemented at a sufficiently low level as you've described above, 
sounds like there's a good chance it'd Just Work for this part too.

>   The linker doesn't need to fix anything up, it just ignores the .dwo 
> sections.

*nod* Not suggesting the linker could/would be able to fix anything up here - 
but that the compiler might need extra command line features to help ensure the 
values were written in a way that was useful.

For example, I believe the Chromium build system may use 
-fdebug-compilation-dir and -fdebug-prefix-map to make cached distributed 
builds work (where it'd be impossible to bake in any particular path - because 
two users may be building Chromium from different locations, but they should be 
able to share build artefacts for maximal caching). But if that's not the usual 
problem/solution (or there are sufficiently many users/use cases that don't 
have that problem) - that's cool & it's nice if this just works for those users 
- and if, for whatever reason, that a Chromium-like situation comes up & wants 
non-split-split-DWARF & needs to address the problem, we can cross that bridge 
at that time (either by using the existing properties (like 
-fdebug-compilation-dir and -fdebug-prefix-map) if they provide enough 
information, or by adding other flags to that family to allow the user to do so)


https://reviews.llvm.org/D52296



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D53334#1273877, @whisperity wrote:

> @dblaikie I have created a test, but unfortunately `%clang_cpp` in LIT 
> invokes `clang --driver-mode=cpp` which is not the same as if `clang++` is 
> called. I'm trying to construct the following command-line
>
> `clang++ -fmodules-ts -fprebuilt-module-path=%t/mods --precompile -c 
> file.cppm -o file.pcm`
>
> However, using `%clang_cc1` I can't get it to accept `--precompile` as a 
> valid argument, and using `%clang_cpp` I get an "unused argument" warning for 
> `--precompile` and the output file is just a preprocessed output (like `-E`), 
> which will, of course, cause errors, but not the errors I am wanting to test 
> about.


Hey, sorry for the delay - you can use "clang -### " (or 
"clang++ -### " to get clang to print out the underlying -cc1 
command line that is used.

If you're changing the driver then we'd write a driver test (that tests that, 
given "clang -foo -###" it produces some "clang -cc1 -bar" command line to run 
the frontend.

But since you're changing the driver, it's not interesting to (re) test how 
--precompile is lowered from the driver to cc1. Instead we test the frontend 
(cc1) directly.


Repository:
  rC Clang

https://reviews.llvm.org/D53334



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D53329#1270035, @yonghong-song wrote:

> Sure. Let me provide a little bit more context and what I want to achieve:
>
>   . I have a tool, called bcc (https://github.com/iovisor/bcc) which uses 
> clang CompilerInvocation interface and
> MCJIT to generates BPF code and load into kernel
>   . Several files (the main.c and a few others headers) are clang memory 
> mapped.
>   . The particular fix here is related to https://reviews.llvm.org/D53261, 
> getting source code into BTF, but
> before that, based on that particular implementation, the source code 
> needs to be in IR.
> What I found is that for the memory mapped /virtual/main.c file, there is 
> one DIFile entry in
> generated IR, the associated 'source' is empty, which actually caused a 
> seg fault later.
>   
> Not that this bug itself does not need https://reviews.llvm.org/D53261.
>   
>
> So without this fix, bcc tool will seg fault.
>  With this fix, bcc tool works properly and all DIFile entry has proper 
> source codes embedded, which
>  if coupled with IR->BTF or Dwarf->BTF should generate correct BTF debug info.


Thanks for the explanation/context.

Any chance of a test case? I'm not sure how the current virtual filesystem 
support is tested & whether it could be extended/used to cover this issue.


Repository:
  rC Clang

https://reviews.llvm.org/D53329



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Thanks! - though on reflection I'm going to invoke @echristo again about the 
naming. It's unfortunately a bit backwards that the pre-standard flag is 
-gsplit-dwarf and what we're proposing as the standard flag is -fdwarf-fission, 
when the DWARF standard doesn't use the word "Fission" at all (only "split 
DWARF").

Maybe it'd be easy enough/better to add the "=single" suffix to the existing 
"-gsplit-dwarf"? (as much as I'm still a bit confused by which debug options 
use a '-g' and which ones use a '-f', etc).

Really sorry to go back/forth/around about on this, George.




Comment at: lib/Driver/ToolChains/Clang.cpp:2999-3001
+StringRef Value = A->getOption().matches(options::OPT_fdwarf_fission_EQ)
+  ? A->getValue()
+  : "split";

Rather than creating a string in the case where there's no need for string 
matching/parsing, perhaps that could be avoided?

  if (!A->getOption().matches(options::OPT_fdwarf_fission_EQ))
return DwarfFissionKind::Split;

  StringRef Value = A->getValue();
  if (Value == "split")
  ...





Comment at: lib/Driver/ToolChains/Clang.cpp:3010-3011
+  }
+  if (ArgIndex)
+*ArgIndex = 0;
+  return DwarfFissionKind::None;

I'd probably either accept this as a precondition (assert(!ArgIndex || ArgIndex 
== 0)) or do this bit at the start of the function rather than the end here, to 
make it simpler/clearer that all paths assign some value to ArgIndex before 
exit of the function (even though that does mean checking/assigning ArgIndex 
unnecessarily in the case where the argument is given, etc)



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

Could having more than one call to getDebugFissionKind lead to the error 
message (for -fdwarf-fission=garbage) being printed multiple times? Or is this 
call on a distinct/mutually exclusive codepath to the other?


https://reviews.llvm.org/D52296



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

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

[David Blaikie via Phabricator via cfe-commits]

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

While I'm not 100% sure about the actual fix - I'm confident enough that 
@rsmith can provide any further clarification in post-commit.

the test case can probably be simplified before commit - I'd suggest removing 
the "positive" case (the case without any diagnostics) as that's likely already 
covered in other tests that added this functionality to begin with, so I'd only 
have the case which produces the diagnostics - that should simplify/reduce the 
RUN lines, remove some of the #ifdefs, etc. (& potentially, you could roll the 
use and definition of the module into the same file as the RUN lines - so it's 
easier to eyeball all the relevant stuff in one place) & remove the function 
from the module - as this seems to produce a diagnostic even if the module is 
empty? Ultimately something like:

  RUN: compile -DMOD_DEF to %t/module_mismatch.pcm
  RUN: compile -DMOD_USE with prebuilt-module-path=%t
  
  #ifdef MOD_DEF
  export module module_mismatch
  #endif
  #ifdef MOD_USE
  import module_mismatch
  #endif

(also removing the extra "prebuilt_modules" directory name to reduce the length 
of command line arguments, etc)


Repository:
  rC Clang

https://reviews.llvm.org/D53334



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

[PATCH] D54243: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

[David Blaikie via Phabricator via cfe-commits]

dblaikie created this revision.
dblaikie added reviewers: JDevlieghere, aprantl, probinson.
Herald added a subscriber: cfe-commits.

This saves a lot of relocations in optimized object files (at the cost
of some cost/increase in linked executable bytes), but gold's 32 bit
gdb-index support has a bug (
https://sourceware.org/bugzilla/show_bug.cgi?id=21894 ) so we can't
switch to this unconditionally. (& even if it weren't for that bug, one
might argue that some users would want to optimize in one direction or
the other - prioritizing object size or linked executable size)

Mostly sending this for review to bikeshed the flag name. (let's
bikeshed the IR names in the LLVM code review)

-fdwarf-base-address is my first prototype. Issues:

- Some existing flags (like -fdebug-types-section) use 'debug' rather than 
'dwarf', but Clang supports formats other than dwarf, so perhaps names related 
to dwarf-specific features should have dwarf in the name, though it's unlikely 
GCC would ever be consistent with that goal?

- base addresses are part of DWARF even beyond this change (eg: range lists are 
relative to the CU's base address - which is somewhat unrelated to the base 
address specifiers used in the range list). So should this specifically call 
out that it's related to ranges? that it's base address /specifiers/ that are 
in use?


Repository:
  rC Clang

https://reviews.llvm.org/D54243

Files:
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -651,6 +651,7 @@
   : Args.hasArg(OPT_gpubnames)
 ? llvm::DICompileUnit::DebugNameTableKind::Default
 : llvm::DICompileUnit::DebugNameTableKind::None);
+  Opts.DebugBaseAddress = Args.hasArg(OPT_fdwarf_base_address);
 
   setPGOInstrumentor(Opts, Args, Diags);
   Opts.InstrProfileOutput =
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3188,6 +3188,11 @@
 ? "-gpubnames"
 : "-ggnu-pubnames");
 
+  if (Args.hasFlag(options::OPT_fdwarf_base_address,
+   options::OPT_fno_dwarf_base_address, false)) {
+CmdArgs.push_back("-fdwarf-base-address");
+  }
+
   // -gdwarf-aranges turns on the emission of the aranges section in the
   // backend.
   // Always enabled for SCE tuning.
Index: include/clang/Frontend/CodeGenOptions.def
===
--- include/clang/Frontend/CodeGenOptions.def
+++ include/clang/Frontend/CodeGenOptions.def
@@ -331,6 +331,9 @@
 /// Whether to emit .debug_gnu_pubnames section instead of .debug_pubnames.
 CODEGENOPT(DebugNameTable, 2, 0)
 
+/// Whether to use DWARF base address specifiers in .debug_ranges.
+CODEGENOPT(DebugBaseAddress, 1, 0)
+
 CODEGENOPT(NoPLT, 1, 0)
 
 /// Whether to embed source in DWARF debug line section.
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1780,6 +1780,10 @@
   Flags<[CC1Option]>, HelpText<"Place debug types in their own section (ELF 
Only)">;
 def fno_debug_types_section: Flag<["-"], "fno-debug-types-section">, 
Group,
   Flags<[CC1Option]>;
+def fdwarf_base_address: Flag <["-"], "fdwarf-base-address">, Group,
+  Flags<[CC1Option]>, HelpText<"Use DWARF base address selection entries in 
debug_ranges">;
+def fno_dwarf_base_address: Flag <["-"], "fno-dwarf_base_address">, 
Group,
+  Flags<[CC1Option]>;
 def fsplit_dwarf_inlining: Flag <["-"], "fsplit-dwarf-inlining">, 
Group,
   Flags<[CC1Option]>, HelpText<"Provide minimal debug info in the 
object/executable to facilitate online symbolication/stack traces in the 
absence of .dwo/.dwp files when using Split DWARF">;
 def fno_split_dwarf_inlining: Flag<["-"], "fno-split-dwarf-inlining">, 
Group,


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -651,6 +651,7 @@
   : Args.hasArg(OPT_gpubnames)
 ? llvm::DICompileUnit::DebugNameTableKind::Default
 : llvm::DICompileUnit::DebugNameTableKind::None);
+  Opts.DebugBaseAddress = Args.hasArg(OPT_fdwarf_base_address);
 
   setPGOInstrumentor(Opts, Args, Diags);
   Opts.InstrProfileOutput =
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3188,6 +3188,11 @@
 ? "-gpubnames

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

[David Blaikie via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346439: [Frontend/Modules] Show diagnostics on prebuilt 
module configuration mismatch… (authored by dblaikie, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53334?vs=173124&id=173212#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53334

Files:
  cfe/trunk/lib/Frontend/CompilerInstance.cpp
  cfe/trunk/test/Modules/mismatch-diagnostics.cpp


Index: cfe/trunk/lib/Frontend/CompilerInstance.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInstance.cpp
+++ cfe/trunk/lib/Frontend/CompilerInstance.cpp
@@ -1727,7 +1727,9 @@
 // module cache, we don't know how to rebuild modules.
 unsigned ARRFlags = Source == ModuleCache ?
 ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
-ASTReader::ARR_ConfigurationMismatch;
+Source == PrebuiltModulePath ?
+0 :
+ASTReader::ARR_ConfigurationMismatch;
 switch (ModuleManager->ReadAST(ModuleFileName,
Source == PrebuiltModulePath
? serialization::MK_PrebuiltModule
Index: cfe/trunk/test/Modules/mismatch-diagnostics.cpp
===
--- cfe/trunk/test/Modules/mismatch-diagnostics.cpp
+++ cfe/trunk/test/Modules/mismatch-diagnostics.cpp
@@ -0,0 +1,22 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/prebuilt_modules
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple  \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-module-interface -pthread -DBUILD_MODULE  \
+// RUN: %s -o %t/prebuilt_modules/mismatching_module.pcm
+//
+// RUN: not %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
+// RUN: %s 2>&1 | FileCheck %s
+
+#ifdef BUILD_MODULE
+export module mismatching_module;
+#endif
+
+#ifdef CHECK_MISMATCH
+import mismatching_module;
+// CHECK: error: POSIX thread support was enabled in PCH file but is currently 
disabled
+// CHECK-NEXT: module file {{.*}}/mismatching_module.pcm cannot be loaded due 
to a configuration mismatch with the current compilation
+#endif
+


Index: cfe/trunk/lib/Frontend/CompilerInstance.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInstance.cpp
+++ cfe/trunk/lib/Frontend/CompilerInstance.cpp
@@ -1727,7 +1727,9 @@
 // module cache, we don't know how to rebuild modules.
 unsigned ARRFlags = Source == ModuleCache ?
 ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
-ASTReader::ARR_ConfigurationMismatch;
+Source == PrebuiltModulePath ?
+0 :
+ASTReader::ARR_ConfigurationMismatch;
 switch (ModuleManager->ReadAST(ModuleFileName,
Source == PrebuiltModulePath
? serialization::MK_PrebuiltModule
Index: cfe/trunk/test/Modules/mismatch-diagnostics.cpp
===
--- cfe/trunk/test/Modules/mismatch-diagnostics.cpp
+++ cfe/trunk/test/Modules/mismatch-diagnostics.cpp
@@ -0,0 +1,22 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/prebuilt_modules
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple  \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-module-interface -pthread -DBUILD_MODULE  \
+// RUN: %s -o %t/prebuilt_modules/mismatching_module.pcm
+//
+// RUN: not %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
+// RUN: %s 2>&1 | FileCheck %s
+
+#ifdef BUILD_MODULE
+export module mismatching_module;
+#endif
+
+#ifdef CHECK_MISMATCH
+import mismatching_module;
+// CHECK: error: POSIX thread support was enabled in PCH file but is currently disabled
+// CHECK-NEXT: module file {{.*}}/mismatching_module.pcm cannot be loaded due to a configuration mismatch with the current compilation
+#endif
+
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

OK - thanks for that.

I'm going to make an executive decision on the naming. Let's go with 
-gsplit-dwarf[=single] (or explicitly -gsplit-dwarf=split, which is the default 
value when -gsplit-dwarf is specified). Saves adding a new name/flag, avoids 
the use of the word Fission which isn't a standard term & so best avoided as a 
way of talking about configuring DWARF standardized functionality.

Could you make that change? & I'll give it another look over after that, but 
should be fine.


https://reviews.llvm.org/D52296



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

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: include/clang/AST/Expr.h:1700-1701
   bool containsNonAscii() const {
-StringRef Str = getString();
-for (unsigned i = 0, e = Str.size(); i != e; ++i)
-  if (!isASCII(Str[i]))
+for (auto c : getString())
+  if (!isASCII(c))
 return true;

Seems like changes like this are unrelated refactoring? Or are they in some way 
needed for this change? 

(also the isXXX functions above - might be neat/tidy to pull those out 
separately (as an NFC change) so that then this change (that modifies the 
implementation of getKind) is just that, without having to refactor all the 
other bits too? But not a big deal either way there, really)


Repository:
  rC Clang

https://reviews.llvm.org/D54166



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

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+if (StringRef(A->getValue()) == "single")
+  return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
 SmallString<128> T(FinalOutput->getValue());

If this function now takes the output file name - could it be simplified to 
just always use that, rather than the conditional code that follows and tests 
whether -o is specified and builds up something like the output file name & 
uses the dwo suffix?



Comment at: test/CodeGen/split-debug-single-file.c:12
+//  RUN:   -enable-split-dwarf=split -split-dwarf-file %t.o -emit-obj -o %t.o 
%s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck 
--check-prefix=MODE-SPLIT %s
+//  MODE-SPLIT-NOT: .dwo

This looks like an end-to-end test, which we don't usually do in Clang (or in 
the LLVM project in general).

For example, the previous testing for split-dwarf had a driver test (that 
tested only that the driver produced the right cc1 invocation) and a CodeGen 
test (that tested that the right IR was produced), but I don't see any test 
like this (that tested the resulting object file)?

I know there's a narrow gap in IR testing - things that don't go in the IR, but 
instead go through MCOptions  & that the SplitDwarfFile is one of those.

So, yeah, in this case it's a bit of a test hole - if you're going to keep this 
test, perhaps it could test assembly, rather than the object file? Since it 
doesn't need complex parsing, etc, perhaps?


https://reviews.llvm.org/D52296



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

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D54166#1295730, @riccibruno wrote:

> @dblaikie Thanks for looking at this patch !
>
> I have a set of patches shrinking the other statements/expressions.
>  Can I add you to review some of these too ? Most of them consists of just 
> moving
>  some data. I added rsmith but I think that he is busy with the standard.


Sure thing! do pull out as many bits of cleanup, renaming, refactoring, etc, 
into separate patches (no worries if you have to also send those for review 
(not sure if you have commit access/what level of stuff you want to 
review-after-commit, etc) - still much easier to review in little pieces than 
all together)


Repository:
  rC Clang

https://reviews.llvm.org/D54166



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

[PATCH] D54399: Move ExprMutationAnalyzer to Tooling/Analysis (1/3)

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Could you fix the modulemap (might amount to reverting the change Eric made in 
r342827? or maybe it's more involved than that) & validate that the modules 
build is working with this change (probably undo Eric's change, validate that 
you see the breakage that Eric was trying to fix, then apply your change & see 
if it clears up as well)?


Repository:
  rC Clang

https://reviews.llvm.org/D54399



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Hey @rjmccall - I'm trying to remember, but can't get it quite clear in my 
head. I seem to recall some discussion about trivial_abi not implying/being 
strong enough for all the cases that trivial_relocatable sounds like it covers? 
Do you happen to remember the distinction there (the summary in this patch 
("the warranting attribute [[trivially_relocatable]], which is similar in 
spirit to [[trivial_abi]], in that it lets the programmer communicate back to 
the compiler that a certain user-defined type should be assumed to have this 
property even though it would not naturally have the property all else being 
equal.") doesn't sound like what I remember - but my memory is hazy)?


Repository:
  rC Clang

https://reviews.llvm.org/D50119



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

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[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/Driver/ToolChains/CommonArgs.cpp:813-830
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+if (StringRef(A->getValue()) == "single")
+  return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
 SmallString<128> T(FinalOutput->getValue());

grimar wrote:
> grimar wrote:
> > dblaikie wrote:
> > > If this function now takes the output file name - could it be simplified 
> > > to just always use that, rather than the conditional code that follows 
> > > and tests whether -o is specified and builds up something like the output 
> > > file name & uses the dwo suffix?
> > I am investigating this.
> So what I see now is that when the function becomes:
> 
> ```
> const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
>   const InputInfo &Output) {
>   SmallString<128> T(Output.getFilename());
>   llvm::sys::path::replace_extension(T, "dwo");
>   return Args.MakeArgString(T);
> }
> ```
> 
> Then no clang tests (check-clang) fail. This does not seem normal to me.
> I would expect that such a change should break some `-fdebug-compilation-dir` 
> relative 
> logic at least and I suspect we just have no test(s) atm.
> 
> What about if I add test case(s) and post a follow-up refactor patch for this 
> place?
> (I started work on it, but it will take some time).
Sounds good - thanks!



Comment at: test/CodeGen/split-debug-single-file.c:12
+//  RUN:   -enable-split-dwarf=split -split-dwarf-file %t.o -emit-obj -o %t.o 
%s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck 
--check-prefix=MODE-SPLIT %s
+//  MODE-SPLIT-NOT: .dwo

grimar wrote:
> dblaikie wrote:
> > This looks like an end-to-end test, which we don't usually do in Clang (or 
> > in the LLVM project in general).
> > 
> > For example, the previous testing for split-dwarf had a driver test (that 
> > tested only that the driver produced the right cc1 invocation) and a 
> > CodeGen test (that tested that the right IR was produced), but I don't see 
> > any test like this (that tested the resulting object file)?
> > 
> > I know there's a narrow gap in IR testing - things that don't go in the IR, 
> > but instead go through MCOptions  & that the SplitDwarfFile is one of those.
> > 
> > So, yeah, in this case it's a bit of a test hole - if you're going to keep 
> > this test, perhaps it could test assembly, rather than the object file? 
> > Since it doesn't need complex parsing, etc, perhaps?
> > So, yeah, in this case it's a bit of a test hole - if you're going to keep 
> > this test, perhaps it could test assembly, rather than the object file? 
> > Since it doesn't need complex parsing, etc, perhaps?
> 
> I am not sure assembly can help here. For example
> `clang main.c -S -g -gsplit-dwarf` would create single asm file output anyways
> and what I tried to check here is that we can either place .dwo sections into 
> the
> same output or into a different one depending on new option.
> 
> > For example, the previous testing for split-dwarf had a driver test (that 
> > tested only that the driver produced the right cc1 invocation) and a 
> > CodeGen test (that tested that the right IR was produced), but I don't see 
> > any test like this (that tested the resulting object file)?
> 
> I think `split-debug-filename.c` do that?
> See: 
> https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/split-debug-filename.c#L18
> 
> Also, `relax.c`: 
> https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/relax.c
> And `mips-inline-asm-abi.c`:  
> https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/mips-inline-asm-abi.c
> 
> Seems it is not common, but still acceptable?
Ah, I see/good point, split-debug-filename.c tests both the IR & then also 
tests the object headers.

Sounds good


https://reviews.llvm.org/D52296



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

[PATCH] D54243: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

[David Blaikie 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 rL346789: DebugInfo: Add a driver flag for DWARF debug_ranges 
base address specifier use. (authored by dblaikie, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54243?vs=173096&id=173905#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54243

Files:
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Frontend/CodeGenOptions.def
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGen/debug-info-ranges-base-address.c
  cfe/trunk/test/Driver/debug-options.c

Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -651,6 +651,7 @@
   : Args.hasArg(OPT_gpubnames)
 ? llvm::DICompileUnit::DebugNameTableKind::Default
 : llvm::DICompileUnit::DebugNameTableKind::None);
+  Opts.DebugRangesBaseAddress = Args.hasArg(OPT_fdebug_ranges_base_address);
 
   setPGOInstrumentor(Opts, Args, Diags);
   Opts.InstrProfileOutput =
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -586,7 +586,8 @@
   CGM.getTarget().getTriple().isNVPTX()
   ? llvm::DICompileUnit::DebugNameTableKind::None
   : static_cast(
-CGOpts.DebugNameTable));
+CGOpts.DebugNameTable),
+  CGOpts.DebugRangesBaseAddress);
 }
 
 llvm::DIType *CGDebugInfo::CreateType(const BuiltinType *BT) {
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3188,6 +3188,11 @@
 ? "-gpubnames"
 : "-ggnu-pubnames");
 
+  if (Args.hasFlag(options::OPT_fdebug_ranges_base_address,
+   options::OPT_fno_debug_ranges_base_address, false)) {
+CmdArgs.push_back("-fdebug-ranges-base-address");
+  }
+
   // -gdwarf-aranges turns on the emission of the aranges section in the
   // backend.
   // Always enabled for SCE tuning.
Index: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
===
--- cfe/trunk/include/clang/Frontend/CodeGenOptions.def
+++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def
@@ -331,6 +331,9 @@
 /// Whether to emit .debug_gnu_pubnames section instead of .debug_pubnames.
 CODEGENOPT(DebugNameTable, 2, 0)
 
+/// Whether to use DWARF base address specifiers in .debug_ranges.
+CODEGENOPT(DebugRangesBaseAddress, 1, 0)
+
 CODEGENOPT(NoPLT, 1, 0)
 
 /// Whether to embed source in DWARF debug line section.
Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1780,6 +1780,10 @@
   Flags<[CC1Option]>, HelpText<"Place debug types in their own section (ELF Only)">;
 def fno_debug_types_section: Flag<["-"], "fno-debug-types-section">, Group,
   Flags<[CC1Option]>;
+def fdebug_ranges_base_address: Flag <["-"], "fdebug-ranges-base-address">, Group,
+  Flags<[CC1Option]>, HelpText<"Use DWARF base address selection entries in debug_ranges">;
+def fno_debug_ranges_base_address: Flag <["-"], "fno-debug-ranges-base-address">, Group,
+  Flags<[CC1Option]>;
 def fsplit_dwarf_inlining: Flag <["-"], "fsplit-dwarf-inlining">, Group,
   Flags<[CC1Option]>, HelpText<"Provide minimal debug info in the object/executable to facilitate online symbolication/stack traces in the absence of .dwo/.dwp files when using Split DWARF">;
 def fno_split_dwarf_inlining: Flag<["-"], "fno-split-dwarf-inlining">, Group,
Index: cfe/trunk/test/Driver/debug-options.c
===
--- cfe/trunk/test/Driver/debug-options.c
+++ cfe/trunk/test/Driver/debug-options.c
@@ -165,6 +165,10 @@
 // RUN: %clang -### -c -gsplit-dwarf %s 2>&1 | FileCheck -check-prefix=GPUB %s
 // RUN: %clang -### -c -gsplit-dwarf -gno-pubnames %s 2>&1 | FileCheck -check-prefix=NOPUB %s
 //
+// RUN: %clang -### -c -fdebug-ranges-base-address %s 2>&1 | FileCheck -check-prefix=RNGBSE %s
+// RUN: %clang -### -c %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
+// RUN: %clang -### -c -fdebug-ranges-base-address -fno-debug-ranges-base-address %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
+//
 // RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=GPUB %s
 // RUN: %clang -### -c -glld

[PATCH] D54243: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Settled on renaming the flag for consistency with, say, -fdebug-types-section, 
to -fdebug-ranges-base-address (though 'debug' is sort of ambiguous (Clang can 
produce non-DWARF debug info) but this driver (as opposed to clang-cl) is 
intended for DWARF emission, not PDB emission - and the "debug-ranges" part of 
the flag names the literal debug_ranges section (like debug-types refers to the 
debug_types section)).


Repository:
  rL LLVM

https://reviews.llvm.org/D54243



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: docs/LanguageExtensions.rst:1096
+  equivalent to copying the underlying bytes and then dropping the source 
object
+  on the floor.
 * ``__is_destructible`` (MSVC 2013)

Quuxplusone wrote:
> rjmccall wrote:
> > Quuxplusone wrote:
> > > rjmccall wrote:
> > > > Quuxplusone wrote:
> > > > > rjmccall wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > Quuxplusone wrote:
> > > > > > > > > @rjmccall wrote:
> > > > > > > > > > trivial_abi permits annotated types to be passed and 
> > > > > > > > > > returned in registers, which is ABI-breaking. Skimming the 
> > > > > > > > > > blog post, it looks like trivially_relocatable does not 
> > > > > > > > > > permit this — it merely signifies that destruction is a 
> > > > > > > > > > no-op after a move construction or assignment.
> > > > > > > > > 
> > > > > > > > > Not necessarily a "no-op"; my canonical example is a 
> > > > > > > > > CopyOnlyCXX03SharedPtr which increments a refcount on 
> > > > > > > > > construction and decrements on destruction. But 
> > > > > > > > > move-construction plus destruction should "balance out" and 
> > > > > > > > > result in no observable side effects.
> > > > > > > > > 
> > > > > > > > > > This is usefully different in the design space, since it 
> > > > > > > > > > means you can safely add the attribute retroactively to 
> > > > > > > > > > e.g. std::unique_ptr, and other templates can then detect 
> > > > > > > > > > that std::unique_ptr is trivially-relocatable and optimize 
> > > > > > > > > > themselves to use memcpy or realloc or whatever it is that 
> > > > > > > > > > they want to do. So in that sense trivial_abi is a 
> > > > > > > > > > *stronger* attribute, not a *weaker* one: the property it 
> > > > > > > > > > determines ought to imply trivially_relocatable.
> > > > > > > > > 
> > > > > > > > > `trivial_abi` is an "orthogonal" attribute: you can have 
> > > > > > > > > `trivial_abi` types with non-trivial constructors and 
> > > > > > > > > destructors, which can have observable side effects. For 
> > > > > > > > > example,
> > > > > > > > > ```
> > > > > > > > > struct [[clang::trivial_abi]] DestructionAnnouncer {
> > > > > > > > > ~DestructionAnnouncer() { puts("hello!"); }
> > > > > > > > > };
> > > > > > > > > ```
> > > > > > > > > is `trivial_abi` (because of the annotation) yet not 
> > > > > > > > > trivially relocatable, because its "move plus destroy" 
> > > > > > > > > operation has observable side effects.
> > > > > > > > > 
> > > > > > > > > > The only interesting question in the language design that I 
> > > > > > > > > > know of is what happens if you put the attribute on a 
> > > > > > > > > > template that's instantiated to contain a sub-object that 
> > > > > > > > > > is definitely not trivially relocatable / trivial-ABI. For 
> > > > > > > > > > trivial_abi, we decided that the attribute is simply 
> > > > > > > > > > ignored — it implicitly only applies to specializations 
> > > > > > > > > > where the attribute would be legal. I haven't dug into the 
> > > > > > > > > > design enough to know what trivially_relocatable decides in 
> > > > > > > > > > this situation, but the three basic options are:
> > > > > > > > > >
> > > > > > > > > > - the attribute always has effect and allows trivial 
> > > > > > > > > > relocation regardless of the subobject types; this is 
> > > > > > > > > > obviously unsafe, so it limits the safe applicability of 
> > > > > > > > > > the attribute to templates
> > > > > > > > > > - the attribute is ignored, like trivial_abi is
> > > > > > > > > > - the attribute is ill-formed, and you'll need to add a 
> > > > > > > > > > [[trivially_relocatable(bool)]] version to support templates
> > > > > > > > > 
> > > > > > > > > What happens is basically the first thing you said, except 
> > > > > > > > > that I disagree that it's "obviously unsafe." Right now, 
> > > > > > > > > conditionally trivial relocation is possible via template 
> > > > > > > > > metaprogramming; see the libcxx patch at e.g.
> > > > > > > > > https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
> > > > > > > > > Since the attribute is an opt-in mechanism, it makes perfect 
> > > > > > > > > sense to me that if you put it on a class (or class 
> > > > > > > > > template), then it applies to the class, without any further 
> > > > > > > > > sanity-checking by the compiler. The compiler has no reason 
> > > > > > > > > to second-guess the programmer here.
> > > > > > > > > 
> > > > > > > > > However, there's one more interesting case. Suppose the 
> > > > > > > > > programmer puts the attribute on a class that isn't 
> > > > > > > > > relocatable at all! (For example, the union case @erichkeane 
> > > > > > > > > mentioned, or a class type with a deleted destructor.) In 
> > > > > > > > > that case, this patch *does* give an error... *unless* the 
> > >

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: docs/LanguageExtensions.rst:1096
+  equivalent to copying the underlying bytes and then dropping the source 
object
+  on the floor.
 * ``__is_destructible`` (MSVC 2013)

Quuxplusone wrote:
> dblaikie wrote:
> > Quuxplusone wrote:
> > > rjmccall wrote:
> > > > Quuxplusone wrote:
> > > > > rjmccall wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > Quuxplusone wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > Quuxplusone wrote:
> > > > > > > > > > > @rjmccall wrote:
> > > > > > > > > > > > trivial_abi permits annotated types to be passed and 
> > > > > > > > > > > > returned in registers, which is ABI-breaking. Skimming 
> > > > > > > > > > > > the blog post, it looks like trivially_relocatable does 
> > > > > > > > > > > > not permit this — it merely signifies that destruction 
> > > > > > > > > > > > is a no-op after a move construction or assignment.
> > > > > > > > > > > 
> > > > > > > > > > > Not necessarily a "no-op"; my canonical example is a 
> > > > > > > > > > > CopyOnlyCXX03SharedPtr which increments a refcount on 
> > > > > > > > > > > construction and decrements on destruction. But 
> > > > > > > > > > > move-construction plus destruction should "balance out" 
> > > > > > > > > > > and result in no observable side effects.
> > > > > > > > > > > 
> > > > > > > > > > > > This is usefully different in the design space, since 
> > > > > > > > > > > > it means you can safely add the attribute retroactively 
> > > > > > > > > > > > to e.g. std::unique_ptr, and other templates can then 
> > > > > > > > > > > > detect that std::unique_ptr is trivially-relocatable 
> > > > > > > > > > > > and optimize themselves to use memcpy or realloc or 
> > > > > > > > > > > > whatever it is that they want to do. So in that sense 
> > > > > > > > > > > > trivial_abi is a *stronger* attribute, not a *weaker* 
> > > > > > > > > > > > one: the property it determines ought to imply 
> > > > > > > > > > > > trivially_relocatable.
> > > > > > > > > > > 
> > > > > > > > > > > `trivial_abi` is an "orthogonal" attribute: you can have 
> > > > > > > > > > > `trivial_abi` types with non-trivial constructors and 
> > > > > > > > > > > destructors, which can have observable side effects. For 
> > > > > > > > > > > example,
> > > > > > > > > > > ```
> > > > > > > > > > > struct [[clang::trivial_abi]] DestructionAnnouncer {
> > > > > > > > > > > ~DestructionAnnouncer() { puts("hello!"); }
> > > > > > > > > > > };
> > > > > > > > > > > ```
> > > > > > > > > > > is `trivial_abi` (because of the annotation) yet not 
> > > > > > > > > > > trivially relocatable, because its "move plus destroy" 
> > > > > > > > > > > operation has observable side effects.
> > > > > > > > > > > 
> > > > > > > > > > > > The only interesting question in the language design 
> > > > > > > > > > > > that I know of is what happens if you put the attribute 
> > > > > > > > > > > > on a template that's instantiated to contain a 
> > > > > > > > > > > > sub-object that is definitely not trivially relocatable 
> > > > > > > > > > > > / trivial-ABI. For trivial_abi, we decided that the 
> > > > > > > > > > > > attribute is simply ignored — it implicitly only 
> > > > > > > > > > > > applies to specializations where the attribute would be 
> > > > > > > > > > > > legal. I haven't dug into the design enough to know 
> > > > > > > > > > > > what trivially_relocatable decides in this situation, 
> > > > > > > > > > > > but the three basic options are:
> > > > > > > > > > > >
> > > > > > > > > > > > - the attribute always has effect and allows trivial 
> > > > > > > > > > > > relocation regardless of the subobject types; this is 
> > > > > > > > > > > > obviously unsafe, so it limits the safe applicability 
> > > > > > > > > > > > of the attribute to templates
> > > > > > > > > > > > - the attribute is ignored, like trivial_abi is
> > > > > > > > > > > > - the attribute is ill-formed, and you'll need to add a 
> > > > > > > > > > > > [[trivially_relocatable(bool)]] version to support 
> > > > > > > > > > > > templates
> > > > > > > > > > > 
> > > > > > > > > > > What happens is basically the first thing you said, 
> > > > > > > > > > > except that I disagree that it's "obviously unsafe." 
> > > > > > > > > > > Right now, conditionally trivial relocation is possible 
> > > > > > > > > > > via template metaprogramming; see the libcxx patch at e.g.
> > > > > > > > > > > https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
> > > > > > > > > > > Since the attribute is an opt-in mechanism, it makes 
> > > > > > > > > > > perfect sense to me that if you put it on a class (or 
> > > > > > > > > > > class template), then it applies to the class, without 
> > > > > > > > > > > any further sanity-checking by the compiler. The compiler 
> > > > > > > > > > > has no reason to second-guess the programmer here.
> > >

[PATCH] D54526: [AST] Pack BinaryOperator

[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! just a few things that could be cleaned up.




Comment at: include/clang/AST/Expr.h:3220
 
-  SourceLocation getExprLoc() const LLVM_READONLY { return OpLoc; }
-  SourceLocation getOperatorLoc() const { return OpLoc; }
-  void setOperatorLoc(SourceLocation L) { OpLoc = L; }
+  SourceLocation getExprLoc() const { return getOperatorLoc(); }
+  SourceLocation getOperatorLoc() const { return BinaryOperatorBits.OpLoc; }

Lost LLVM_READONLY somewhere along the way? Should it be added back in?



Comment at: include/clang/AST/Expr.h:3256-3259
+  static bool isPtrMemOp(Opcode Opc) {
+return Opc == BO_PtrMemD || Opc == BO_PtrMemI;
+  }
+  bool isPtrMemOp() const { return isPtrMemOp(getOpcode()); }

I probably wouldn't add a new public member function here - I take it you added 
that to avoid calling getOpcode() twice? (either for performance or 
textual/source tersity?)

Calling it twice probably is OK for both performance and readability - but if 
you prefer otherwise, I guess you could do it with a temporary:

  bool isPetrMemOp() const {
auto Opc = getOpcode();
return Opc == ... ;
  }




Comment at: include/clang/AST/Expr.h:3369-3375
   bool isFPContractableWithinStatement() const {
-return FPOptions(FPFeatures).allowFPContractWithinStatement();
+return getFPFeatures().allowFPContractWithinStatement();
   }
 
   // Get the FENV_ACCESS status of this operator. Only meaningful for
   // operations on floating point types.
+  bool isFEnvAccessOn() const { return getFPFeatures().allowFEnvAccess(); }

Could do these two (& some of the other "refactor to use a common accessor, 
because the accessor is getting a bit more interesting") changes as NFC cleanup 
before this - but no big deal, just mention it in case in other/future changes 
that makes sense for you.


Repository:
  rC Clang

https://reviews.llvm.org/D54526



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

[PATCH] D54526: [AST] Pack BinaryOperator

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: include/clang/AST/Stmt.h:572
 CastExprBitfields CastExprBits;
+BinaryOperatorBitfields BinaryOperatorBits;
 InitListExprBitfields InitListExprBits;

Oh, just a thought - wonder if we could/should have an assert that this 
anonymous union doesn't grow beyond a certain intended size? (can we have such 
an assert? I'm not sure how to test the size of an anonymous union - I guess we 
could static_assert the size of the whole Stmt class, though that'd be a bit 
brittle)


Repository:
  rC Clang

https://reviews.llvm.org/D54526



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

[PATCH] D54525: [AST] Pack MemberExpr

[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!




Comment at: include/clang/AST/Expr.h:2799-2802
-
-  friend TrailingObjects;
-  friend class ASTReader;
-  friend class ASTStmtWriter;

Moving these around could/should be a separate NFC commit - did these have to 
move at all in this change or something you spotted along the way & cleaned up 
independently?


Repository:
  rC Clang

https://reviews.llvm.org/D54525



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

[PATCH] D54524: [AST] Pack UnaryOperator

[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/D54524



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Thanks for this - though it looks like the test program hits an assertion 
failure (for me at least - before it gets to the interesting point.

clang-test: 
/usr/local/google/home/blaikie/dev/llvm/src/lib/ExecutionEngine/MCJIT/MCJIT.cpp:204:
 virtual void llvm::MCJIT::generateCodeForModule(llvm::Module *): Assertion 
`M->getDataLayout() == getDataLayout() && "DataLayout Mismatch"' failed.

Also, while this is useful for me to understand what's going on (thanks!) & it 
looks like might lead us to a better understanding of where the bug(s) are - 
this change would also need a standalone test case in clang without the need to 
build a new/separate binary. But I imagine this could be tested in Clang by 
running clang on this source file & checking the IR that clang generates? Or 
does clang not generate the interesting IR itself, only when invoked through 
some other codepath you've demonstrated in the clang-test.cpp does this come up?


Repository:
  rC Clang

https://reviews.llvm.org/D53329



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

[PATCH] D54576: [clang] - Simplify tools::SplitDebugName.

[David Blaikie via Phabricator via cfe-commits]

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

Agreed - looks like this went in untested in r175813 & has never been used.


https://reviews.llvm.org/D54576



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a subscriber: scott.linder.
dblaikie added a comment.

Thanks!

So I can see where/how this fails now - the LLVM backend seems to require that 
if any file has embedded source, they all do. Would you be able to/would you 
mind adding a debug info verifier check for this? That'd help make this fail 
sooner. (@scott.linder - perhaps you'd be able to/interested in doing this - 
looks like you wrote the initial implementation?)

Beyond that, then the question is whether this fix is the right way to satisfy 
that requirement (I'm assuming the requirement is reasonable - though I've not 
looked at the embedded source support & have no idea if there's a way/it's 
reasonable to support some embedded source and some not). My big question & 
I've tried to do some debugging to better understand this - but don't have the 
time to dive much further into it (& haven't really figured it out as yet): Why 
is this source not accessible through this API at this point? is the fallback 
to the main file always accurate?

(as far as I got was figuring out that SourceManager::getBufferData had an SLoc 
with isFile == true for the header, but isFile was false for the SLocs related 
to the .cpp file. Can't say I know why that is/what that means & would like to 
understand that (or someone else more familiar with this stuff who already 
knows can review this code) before approving this patch - if you could help 
explain this, that'd be great)


Repository:
  rC Clang

https://reviews.llvm.org/D53329



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

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Cool - thanks! Any chance/way to add a test for this that'd show up sooner than 
the breakage caused by the previous version?


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

https://reviews.llvm.org/D55006



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

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D55006#1311398 , @grimar wrote:

> In D55006#1311391 , @dblaikie wrote:
>
> > Cool - thanks! Any chance/way to add a test for this that'd show up sooner 
> > than the breakage caused by the previous version?
>
>
> Maybe, I am not sure. I was not able to trigger this without using the 
> `clang-tidy` call yet.


Any chance other clang tools might reproduce this? clang-check maybe (sort of 
the most trivial clang tool)?


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

https://reviews.llvm.org/D55006



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

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

[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 OK to me


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

https://reviews.llvm.org/D55085



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Can't say I know much abouth the path remapping functionality - what it's used 
for, where it's implemented in general, etc - so figure someone with more of 
that knowledge might be best off reviewing this.


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

https://reviews.llvm.org/D55137



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

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added subscribers: labath, klimek.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good to me - tagging @labath @klimek here in case they have some further 
context on whether this is the right place for the test. But I'm fine with them 
providing post-commit review - please go ahead with this change as-is.


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

https://reviews.llvm.org/D55006



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:8979
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (std::get<0>(Pair) == None)
   return Comparison::Worse;

I'd probably write this as "if (!std::get<0>(Pair))" - but I understand that 
opinions differ on such things easily enough.



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

This might be more legible as "if (std::get<0>(Pair) || std::get<1>(Pair))", I 
think? (optionally using "hasValue", if preferred - but comparing the hasValues 
to each other, rather than testing if either is false, seems a bit opaque to me 
at least)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:8979
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (std::get<0>(Pair) == None)
   return Comparison::Worse;

Meinersbur wrote:
> dblaikie wrote:
> > I'd probably write this as "if (!std::get<0>(Pair))" - but I understand 
> > that opinions differ on such things easily enough.
> Here I tried do be explicit that it's an `llvm::Optional` and not testing for 
> null pointers (or whatever the payload of the llvm::Optional is, which might 
> itself have an overload bool conversion operator). It seemed worthwhile 
> especially because `llvm::Optional` itself does not appear itself around 
> these lines.
*nod* Up to you - though, given std::get<0> and std::get<1> appear twice in 
this loop, perhaps it makes sense to pull them out into named variables and 
then you can name the Optional too?



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

Meinersbur wrote:
> dblaikie wrote:
> > This might be more legible as "if (std::get<0>(Pair) || 
> > std::get<1>(Pair))", I think? (optionally using "hasValue", if preferred - 
> > but comparing the hasValues to each other, rather than testing if either is 
> > false, seems a bit opaque to me at least)
> The idea was 'if the items are unequal, the list is unequal', where when 
> either one is undefined, but not the the other, is considered unequal. The 
> test for the elements themselves have the same structure (Cand1ID != Cand2ID).
Sorry, looks like I made a mistake in the suggested change - should be a ! 
before each of the gets (I wonder if the change as you have it now/as I 
originally suggested, is causing any test failures - one really hopes it 
does... because as written it looks like it'd cause this loop to always return 
false on the first iteration):

  if (!std::get<0>(Pair) || !std::get<1>(Pair))

& thanks for the explanation about the approach you were using - I see where 
you're coming from. I'd personally still lean this ^ way, I think.

(maybe if we get super ridiculous one day, and have a monadic (not sure if 
that's the right word) sort of conditional accessor for Optional (where you 
pass in a lambda over T, returning U, and the result is Optional) we could 
write this in terms of that & then the != would fit right in... though would be 
a bit verbose/quirky to be sure)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[David Blaikie via Phabricator via cfe-commits]

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

Overall I'm not sure this construct/pattern improves readability, but not too 
fussed either way.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

Meinersbur wrote:
> dblaikie wrote:
> > Meinersbur wrote:
> > > dblaikie wrote:
> > > > This might be more legible as "if (std::get<0>(Pair) || 
> > > > std::get<1>(Pair))", I think? (optionally using "hasValue", if 
> > > > preferred - but comparing the hasValues to each other, rather than 
> > > > testing if either is false, seems a bit opaque to me at least)
> > > The idea was 'if the items are unequal, the list is unequal', where when 
> > > either one is undefined, but not the the other, is considered unequal. 
> > > The test for the elements themselves have the same structure (Cand1ID != 
> > > Cand2ID).
> > Sorry, looks like I made a mistake in the suggested change - should be a ! 
> > before each of the gets (I wonder if the change as you have it now/as I 
> > originally suggested, is causing any test failures - one really hopes it 
> > does... because as written it looks like it'd cause this loop to always 
> > return false on the first iteration):
> > 
> >   if (!std::get<0>(Pair) || !std::get<1>(Pair))
> > 
> > & thanks for the explanation about the approach you were using - I see 
> > where you're coming from. I'd personally still lean this ^ way, I think.
> > 
> > (maybe if we get super ridiculous one day, and have a monadic (not sure if 
> > that's the right word) sort of conditional accessor for Optional (where you 
> > pass in a lambda over T, returning U, and the result is Optional) we 
> > could write this in terms of that & then the != would fit right in... 
> > though would be a bit verbose/quirky to be sure)
> I knew exactly what you suggested -- I considered before going with the `!=` 
> version -- it seems It also only saw what I wanted to see. I still just 
> copy&pasted from your comment to save some keystrokes. Maybe the `!=` is less 
> error-prone, as just demonstrated? Test cases did not fail.
I'd still probably go with the !A || !B form - and either encourage you to add 
the missing test coverage, or maybe go find who owns/contributed/reviewed the 
code to request/suggest some testing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D57986: [ProfileData] Sort FuncData before iteration to remove non-determinism

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/ProfileData/InstrProfWriter.cpp:389-397
+ auto nameA = A.first;
+ auto nameB = B.first;
+ int comp = nameA.compare(nameB);
+ if (comp)
+   return comp < 0;
+
+ auto hashA = A.second.first;

I guess you could write this as:

  return std::tie(A.first, A.second.first) < std::tie(B.first, B.second.first);

perhaps?


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

https://reviews.llvm.org/D57986



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

[PATCH] D57986: [ProfileData] Sort FuncData before iteration to remove non-determinism

[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/ProfileData/InstrProfWriter.cpp:431-432
+  for (const auto &record : OrderedFuncData) {
+const auto &name = record.first;
+const auto &Func = record.second;
+writeRecordInText(name, Func.first, Func.second, Symtab, OS);

Inconsistent variable naming? (some start with upper case, some with lower case)

Currently I think the LLVM style says upper case is required.

Naming the types here might be reasonable for readability, but I'm not sure.


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

https://reviews.llvm.org/D57986



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Would it be simpler/better to revert all the FlagTrivial work? & use the 
FlagNonTrivial+composite type to imply trivial? (since FlagnonTrivial has been 
in-tree longer)


Repository:
  rC Clang

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

https://reviews.llvm.org/D59347



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Pleasue include mention of the bug (PR40276) in the commit message & clarify 
that while this is useful for some remote compilation models, it's not strictly 
necessary/the only way to do it (a remote compilation model that keeps relative 
paths and uses compilation-dir instead can work without this attribute)

Use llvm-dwarfdump rather than llvm-objdump to dump the contents of the 
debug_info section and test the dwo_name there (rather than dumping hex) & 
probably the objdump part of the test isn't needed? (& I guess there's an LLVM 
patch to add the rest of this functionality?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D59008#1441903 , @t-tye wrote:

> LGTM
>
> Do we know the state of split DWARF and DWARF compression for DWARF 5 
> (compared to DWARF 2)?


State of them in what sense? Compression is pretty orthogonal to any DWARF 
version - it's more about the container (ELF, etc) you use. Split DWARF is 
non-standardly supported in pre-v5, and I think it's functioning in the 
standards conformant v5 mode too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59008



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D59008#1442256 , @t-tye wrote:

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


Might be - nothing I know of right now. (type units aren't fully supported in 
DWARFv5 - but that's the only big thing on my list)


Repository:
  rC Clang

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

https://reviews.llvm.org/D59008



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@asmith: Where's the LLVM-side change/review that goes with this, btw?

In D59347#1442970 , @probinson wrote:

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


Fair enough - I was mostly coming at this from the "the patch that was 
committed should be reverted" & then we could haggle over other things, but 
fair point.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59347



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D59008#1442962 , @probinson wrote:

> In D59008#1442515 , @dblaikie wrote:
>
> > In D59008#1442256 , @t-tye wrote:
> >
> > > In D59008#1442014 , @dblaikie 
> > > wrote:
> > >
> > > > In D59008#1441903 , @t-tye 
> > > > wrote:
> > > >
> > > > > LGTM
> > > > >
> > > > > Do we know the state of split DWARF and DWARF compression for DWARF 5 
> > > > > (compared to DWARF 2)?
> > > >
> > > >
> > > > State of them in what sense? Compression is pretty orthogonal to any 
> > > > DWARF version - it's more about the container (ELF, etc) you use. Split 
> > > > DWARF is non-standardly supported in pre-v5, and I think it's 
> > > > functioning in the standards conformant v5 mode too.
> > >
> > >
> > > I had heard mention that at least split DWARF may not be fully supported 
> > > yet for DWARF 5 in LLVM. But maybe that is old news:-)
> >
> >
> > Might be - nothing I know of right now. (type units aren't fully supported 
> > in DWARFv5 - but that's the only big thing on my list)
>
>
> Anything having to do with section format and placement ought to be correct 
> at this point.  Certainly I see v5 type units coming out in the .debug_info 
> section per spec, and the .dwo files look right.  If there's something 
> missing please file a bug and CC me.
>
> There was certainly a release where split-dwarf and type units didn't work 
> yet, but that's all done now.


Ah, thanks for the correction - I thought that was the case, and then I thought 
I'd tested it a couple of weeks ago and seen it wasn't (when I'd expected it 
was)... must've messed up my test, maybe used an old compiler build I had lying 
around.

Good stuff!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59008



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D59347#1443051 , @dblaikie wrote:

> @asmith: Where's the LLVM-side change/review that goes with this, btw?
>
> In D59347#1442970 , @probinson wrote:
>
> > As a rule I would prefer flags with positive names, as it's slightly easier 
> > to read `!isTrivial` than `!isNonTrivial`. And trivially shorter. :-)
>
>
> Fair enough - I was mostly coming at this from the "the patch that was 
> committed should be reverted" & then we could haggle over other things, but 
> fair point.


Hmm, one other thought: Technically "non trivial" is perhaps more accurate/less 
error prone. Only marking structures as "trivial" but other types without that 
marker makes it more subtle (since not all trivial types would be marked 
trivial - only those of a classification that means they /could/ be 
non-trivial). Whereas marking the non-trivial types is more broadly accurate.

@asmith - is this patch now essentially a revert of the trivial flag addition? 
Or are there any parts that were not reverted, if so, why not? (I would 
expect/imagine all the testing could be reverted too - since the NonTrivial 
flag was presumably already tested appropriately?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D59347



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

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

[David Blaikie via Phabricator via cfe-commits]

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

What's the general motivation for this work/changes?

> -gmlt -gsplit-dwarf -fno-split-dwarf-inlining => special: 1 (before) 2 (after)

This ^ is the only semantic change due to this refactoring?

There's a desire to be able to compose gmlt+gsplit-dwarf, /if/ 
-fno-split-dwarf-inlining is enabled. (for context, -fno-split-dwarf-inlining 
is the default in Google's optimized builds, since split-dwarf-inlining was 
never implemented in GCC & we didn't want to regress object size when switching 
from GCC to Clang (& no one had complained about that missing functionality))

So with -fno-split-dwarf-inlining, there's value in using gmlt with 
gsplit-dwarf (it reduces the size of the .dwo files - reducing the inputs/size 
of dwp, etc).

& it sounds like this change breaks that scenario?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59923



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D59923#1447198 , @MaskRay wrote:

> In D59923#1446508 , @dblaikie wrote:
>
> > What's the general motivation for this work/changes?
>
>
> The current -gsplit-dwarf handling is a bit complex and the motivation is to 
> make it less confusing.
>
>   -g0 -gsplit-dwarf => 2
>   -gmlt -gsplit-dwarf -fsplit-dwarf-inlining => 2
>   -gmlt -gsplit-dwarf -fno-split-dwarf-inlining => 1 (before) 2 (after)
>   
>
> It is confusing because the composition of `-gmlt -gsplit-dwarf` is different 
> from `-g0 -gsplit-dwarf` when `SplitDWARFInlining` is false.
>
> >> -gmlt -gsplit-dwarf -fno-split-dwarf-inlining => special: 1 (before) 2 
> >> (after)
> > 
> > This ^ is the only semantic change due to this refactoring?
>
> This is the only semantic change.
>
> > There's a desire to be able to compose gmlt+gsplit-dwarf, /if/ 
> > -fno-split-dwarf-inlining is enabled. (for context, 
> > -fno-split-dwarf-inlining is the default in Google's optimized builds, 
> > since split-dwarf-inlining was never implemented in GCC & we didn't want to 
> > regress object size when switching from GCC to Clang (& no one had 
> > complained about that missing functionality))
> > 
> > So with -fno-split-dwarf-inlining, there's value in using gmlt with 
> > gsplit-dwarf (it reduces the size of the .dwo files - reducing the 
> > inputs/size of dwp, etc).
> > 
> > & it sounds like this change breaks that scenario?
>
> Acknowledged the desire to compose `-gsplit-dwarf -gmlt 
> -fno-split-dwarf-inlining`. After this patch, users should place `-gmplt` 
> after `-gsplit-dwarf`.
>
> In Bazel, the order of the command line options from left to right: feature 
> flags < BUILD `copts=` < `--copt=`. So for targets specifying `-g0`/`-gmlt` 
> in their `copts` / explict `--copts=` options, they will override 
> `-gsplit-dwarf` added by the debug feature.


OK - could you include/describe which sets of options disable split-dwarf, 
then? (adding that to the patch description along with the matrix of g1/2, etc? 
(specifically, I'd expect "-gmlt -gsplit-dwarf" means 2+split, "-gsplit-dwarf 
-gmlt" means 1+non-split, and "-fno-split-dwarf-inlining -gsplit-dwarf -gmlt" 
(with -fno-split-dwarf-inlining anywhere in the command line (so long as it's 
after an -fsplit-dwarf-inlining) is 1+split)

I'm still not quite sure changing the meaning of "-gmlt -gsplit-dwarf 
-fno-split-dwarf-inlining" is important. It feels to me like in that mode, 
-gmlt and -gsplit-dwarf compose naturally in either order. Is it code 
complexity or user interface complexity you're trying to address? I'm still a 
bit curious/trying to better understand the motivation here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59923



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

[PATCH] D58497: Clear the KnownModules cache if the preprocessor is going away

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D58497#1449243 , @nemanjai wrote:

> Ping.


Unfortunately Richard Smith is out for a few weeks at the moment, so might take 
a little bit before he can get to this.

It's odd to me that this lacks a test case - but you mention it's shown up on 
buildbots? Does it reproduce consistently there? Under what conditions (which 
buildbots/configurations show this - are they permanently failing because of 
this?)?

A test case, if at all possible, would be super helpful.

> If there are no objections in the next week or so, I'll commit this and it 
> can be reviewed post-commit.

That's generally not considered acceptable practice - if something is sent for 
review it's because it needs review & time doesn't change that. (there are some 
exceptions to this - some folks send things out for "hey, anyone got other 
ideas on this, otherwise I think it's fine" sort of thing)


Repository:
  rC Clang

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

https://reviews.llvm.org/D58497



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D59673#143 , @aaronpuchert 
wrote:

> In D59673#1438716 , @dblaikie wrote:
>
> > Use llvm-dwarfdump rather than llvm-objdump to dump the contents of the 
> > debug_info section and test the dwo_name there (rather than dumping hex)
>
>
> I didn't know about llvm-dwarfdump, I wondered why llvm-objdump wouldn't 
> pretty-print the debug info as objdump does. That makes a lot of sense then.
>
> > probably the objdump part of the test isn't needed?
>
> Actually I only need to check that the DWO file is there (under the proper 
> file name). Any ideas?


Ah, fair. You could actually test the dwo_name is accurate in the .dwo file (I 
added the dwo_name to the .dwo file so that multi-level dwp error messages 
could be more informative)

>> (& I guess there's an LLVM patch to add the rest of this functionality?)
> 
> What do we have to do there? Should we add the option to `llc` as well?

Yep - pretty much anything in MCOptions should be surfaced through llc for 
llvm-level testing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

I'm still not entirely clear on how this is all changing, sorry - in the patch 
description summary, the first block of examples doesn't mention which of those 
disables split DWARF (the second block of examples all say "+ split" - is that 
to imply that the first block all do not use split DWARF?

In a previous message I think you said that the only change was "-gmlt 
-gsplit-dwarf -fno-split-dwarf-inlining => 1 (before) 2 (after)" - which I'm 
not sure is an improvement. You mentioned that the inconsistency between "-g0 
-gsplit-dwarf" and "-gmlt -gsplit-dwarf -fno-split-dwarf-inlining" was 
confusing. But still there will be an inconsistency between "-gsplit-dwarf -g0" 
and "-gsplit-dwarf -gmlt -fno-split-dwarf-inlining", yes?

I think that under -fno-split-dwarf-inlining, -gmlt and -gsplit-dwarf should be 
order independent and compositional rather than overriding. Having them compose 
in one order but not the other seems confusing to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59923



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D59923#1460696 , @MaskRay wrote:

> > is that to imply that the first block all do not use split DWARF?
>
> The first block do not use split DWARF.


That doesn't sound like what I'd expect (& would represent a change in behavior 
as well). The first block reads:

-gsplit-dwarf -g0 => 0
-gsplit-dwarf -gline-directives-only => DebugDirectivesOnly
-gsplit-dwarf -gmlt -fsplit-dwarf-inlining => 1
-gsplit-dwarf -gmlt -fno-split-dwarf-inlining => 1

This last one currently produces split-dwarf (if there's any DWARF worth 
splitting - if there are any subprogram descriptions, etc, otherwise it saves 
the indirection and produces an empty .dwo file).

>> In a previous message I think you said that the only change was "-gmlt 
>> -gsplit-dwarf -fno-split-dwarf-inlining => 1 (before) 2 (after)" - which I'm 
>> not sure is an improvement.
> 
> Yes, this is the only behavioral change.
> 
>> You mentioned that the inconsistency between "-g0 -gsplit-dwarf" and "-gmlt 
>> -gsplit-dwarf -fno-split-dwarf-inlining" was confusing. But still there will 
>> be an inconsistency between "-gsplit-dwarf -g0" and "-gsplit-dwarf -gmlt 
>> -fno-split-dwarf-inlining", yes?
> 
> The debug info level will be consistent after this patch:  the last of 
> `-gsplit-dwarf -g0 -g1 -g2 -g3 -ggdb[0-3] -gdwarf-*` will decide the debug 
> info level (`-gsplit-dwarf -gdwarf-*` have level 2). Next, a separate rule 
> decides if the `-gsplit-dwarf` takes effect (not if `DebugInfoKind == 
> codegenoptions::NoDebugInfo || DebugInfoKind == 
> codegenoptions::DebugDirectivesOnly || (DebugInfoKind == 
> codegenoptions::DebugLineTablesOnly && SplitDWARFInlining)`)
> 
>> I think that under -fno-split-dwarf-inlining, -gmlt and -gsplit-dwarf should 
>> be order independent and compositional rather than overriding. Having them 
>> compose in one order but not the other seems confusing to me.
> 
> The existence of `-fno-split-dwarf-inlining` changing the position dependence 
> makes me confused:
> 
> - Without it, the latter of `-gmlt` and `-gsplit-dwarf` decides the debug 
> info level
> - With it, `-gmlt` decides the debug info level

Seems there's going to be confusion either way, though - either the 
presence/absence of -fno-split-dwarf-inlining changes whether -gsplit-dwarf is 
respected/ignored (in the presence of -gmlt), or changes whethe -gmlt composes 
with -gsplit-dwarf or overrides it? Seems these are both a bit confusing, no?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59923



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D59673#1460605 , @aaronpuchert 
wrote:

> Ok, here is an idea. We introduce `-split-dwarf-output` in Clang instead of 
> `-fsplit-dwarf-dwo-name-attr`. If given, it overrides the output file name 
> for the Split DWARF file, which we otherwise take from `-split-dwarf-file`. 
> The option is obviously incompatible with `-enable-split-dwarf=single`, so we 
> will disallow that. This should be backwards-compatible, and bring the 
> behavior of `llc` and `clang -cc1` closer together. What do you think?


Sure, I think the naming's a bit weird (but hard to come up with good names for 
any of this) - but consistency seems like a good first pass at least, given 
we're halfway there anyway.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

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

[David Blaikie via Phabricator via cfe-commits]

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

Approved pending the LLVM side changes/discussion.


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

https://reviews.llvm.org/D59347



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D59673#1465975 , @aaronpuchert 
wrote:

> In D59673#1461983 , @dblaikie wrote:
>
> > Sure, I think the naming's a bit weird (but hard to come up with good names 
> > for any of this)
>
>
> Agreed, `-split-dwarf-output` is pretty clear, but `-split-dwarf-file` could 
> be both the actual filename or the attribute.
>
> There is `test/CodeGen/split-debug-filename.c` checking that we emit 
> `!DICompileUnit({{.*}}, splitDebugFilename: "foo.dwo"` into the IR under some 
> circumstances, but I'm not sure why. It seems to be ignored by `llc`.


For implicit modular debug info I think it probably isn't ignored. See 
DwarfDebug.cpp:~630.

> What's the intended use for this? Your commit message in rC301063 
>  suggests that this is needed for implicit 
> modules. How does this work? I'm asking of course because I'm not sure 
> whether we might need to emit both file names there.

So implicit modules can use a sort of pseudo-split DWARF. The .pcm (module 
file) itself is an object file in this mode - with the split DWARF and a 
section containing the AST bitcode. Then in the normal source files that use 
the .pcm, they get the usual non-split DWARF, plus skeleton CUs that reference 
the .pcm file (& contain some other attributes for regenerating it in case it's 
been cleaned up from the module cache). This is why the file name can be (& has 
to be) passed down through the IR - it /can/ be, because this compilation isn't 
choosing tnhe file name (so there's no issue with the .dwo file name changing 
between IR generation and object generation (as there is with LTO - where the 
compilation doesn't know the final object name, only the linker knows that)) 
and it /must/ be done this way due to multiple skeleton CUs if the main CU 
references multiple .pcm debug info.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

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

[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 - thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59923



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

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Sema/SemaDeclAttr.cpp:8623
+  } else if (A.hasScope()) {
+#define ATTR(A)
+#define ATTR_NAMESPACE(A) .Case(#A, false)

dblaikie wrote:
> Not sure how it's done elsewhere - but I'd sink these #defines down to 
> immediately before the #include
I think most other uses of .inc files only #define the macros they need to - 
assuming the others aren't defined, rather than explicitly providing an empty 
definition? (but I'm not sure - I guess that means teh .inc file needs a 
#ifndef X \ #define X #endif - but perhaps .inc files usually have those?)



Comment at: lib/Sema/SemaDeclAttr.cpp:8623-8624
+  } else if (A.hasScope()) {
+#define ATTR(A)
+#define ATTR_NAMESPACE(A) .Case(#A, false)
+if (llvm::StringSwitch(A.getScopeName()->getName())

Not sure how it's done elsewhere - but I'd sink these #defines down to 
immediately before the #include



Comment at: lib/Sema/SemaDeclAttr.cpp:8629-8630
+  Warning = diag::warn_unknown_attribute_namespace_ignored;
+#undef ATTR_NAMESPACE
+#undef ATTR
+  } else if (A.isC2xAttribute() || A.isCXX11Attribute()) {

Should AttrList.inc handle undefing all its attributes - so all its users don't 
have to?



Comment at: utils/TableGen/ClangAttrEmitter.cpp:2553
 
+void copyAttrNamespaces(std::set &S) const {
+  for (const Record *R : Attrs) {

Maybe "addAttrNamespacesTo" (not clear whether this would overwrite the contens 
of the std::set, or add to it)


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

https://reviews.llvm.org/D60872



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

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

[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 pretty good to me - if you'd like to wait for more/other feedback from 
@rsmith or the like, that's OK too.


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

https://reviews.llvm.org/D60872



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

[PATCH] D60892: Modules: Search for a visible definition of the decl context when computing visibility of a default template parameter

[David Blaikie via Phabricator via cfe-commits]

dblaikie created this revision.
dblaikie added a reviewer: rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The code is/was already correct for the case where a parameter is a
parameter of its enclosing lexical DeclContext (functions and classes).
But for other templates (alias and variable templates) they don't create
their own scope to be members of - in those cases, they parameter should
be considered visible if any definition of the lexical decl context is
visible.

[this should cleanup the failure on the libstdc++ modules buildbot]
[this doesn't actually fix the variable template case for a
secondary/compounding reason (its lexical decl context is incorrectly
considered to be the translation unit)]

Test covers all 4 kinds of templates with default args, including a
regression test for the still broken variable template case.


Repository:
  rC Clang

https://reviews.llvm.org/D60892

Files:
  lib/Sema/SemaLookup.cpp
  test/Modules/Inputs/nested-template-default-arg-redecl/alias.h
  test/Modules/Inputs/nested-template-default-arg-redecl/alias1.h
  test/Modules/Inputs/nested-template-default-arg-redecl/alias2.h
  test/Modules/Inputs/nested-template-default-arg-redecl/func.h
  test/Modules/Inputs/nested-template-default-arg-redecl/func1.h
  test/Modules/Inputs/nested-template-default-arg-redecl/func2.h
  test/Modules/Inputs/nested-template-default-arg-redecl/module.modulemap
  test/Modules/Inputs/nested-template-default-arg-redecl/strct.h
  test/Modules/Inputs/nested-template-default-arg-redecl/strct1.h
  test/Modules/Inputs/nested-template-default-arg-redecl/strct2.h
  test/Modules/Inputs/nested-template-default-arg-redecl/var.h
  test/Modules/Inputs/nested-template-default-arg-redecl/var1.h
  test/Modules/Inputs/nested-template-default-arg-redecl/var2.h
  test/Modules/nested-template-default-arg-redecl.cpp

Index: test/Modules/nested-template-default-arg-redecl.cpp
===
--- /dev/null
+++ test/Modules/nested-template-default-arg-redecl.cpp
@@ -0,0 +1,18 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x c++ -fmodules -fimplicit-module-maps -fmodules-cache-path=%t \
+// RUN: -I %S/Inputs/nested-template-default-arg-redecl -std=c++14 \
+// RUN: -fmodules-local-submodule-visibility -verify %s
+#include "alias2.h"
+#include "var2.h"
+#include "strct2.h"
+#include "func2.h"
+
+// FIXME: Variable templates lexical decl context appears to be the translation
+// unit, which is incorrect. Fixing this will hopefully address the following
+// error/bug:
+
+// expected-note@Inputs/nested-template-default-arg-redecl/var.h:4 {{default argument declared here}}
+auto var = &var_outer::var<>; // expected-error {{default argument of 'var' must be imported from module 'VAR1' before it is required}}
+auto func = &func_outer::func<>;
+strct_outer::strct<> *strct;
+alias_outer::alias<> *alias;
Index: test/Modules/Inputs/nested-template-default-arg-redecl/var2.h
===
--- /dev/null
+++ test/Modules/Inputs/nested-template-default-arg-redecl/var2.h
@@ -0,0 +1 @@
+#include "var.h"
Index: test/Modules/Inputs/nested-template-default-arg-redecl/var1.h
===
--- /dev/null
+++ test/Modules/Inputs/nested-template-default-arg-redecl/var1.h
@@ -0,0 +1 @@
+#include "var.h"
Index: test/Modules/Inputs/nested-template-default-arg-redecl/var.h
===
--- /dev/null
+++ test/Modules/Inputs/nested-template-default-arg-redecl/var.h
@@ -0,0 +1,9 @@
+#ifndef VAR_H
+#define VAR_H
+struct var_outer {
+  template 
+  static int var;
+};
+#endif
+
+
Index: test/Modules/Inputs/nested-template-default-arg-redecl/strct2.h
===
--- /dev/null
+++ test/Modules/Inputs/nested-template-default-arg-redecl/strct2.h
@@ -0,0 +1 @@
+#include "strct.h"
Index: test/Modules/Inputs/nested-template-default-arg-redecl/strct1.h
===
--- /dev/null
+++ test/Modules/Inputs/nested-template-default-arg-redecl/strct1.h
@@ -0,0 +1 @@
+#include "strct.h"
Index: test/Modules/Inputs/nested-template-default-arg-redecl/strct.h
===
--- /dev/null
+++ test/Modules/Inputs/nested-template-default-arg-redecl/strct.h
@@ -0,0 +1,7 @@
+#ifndef STRCT_H
+#define STRCT_H
+struct strct_outer {
+  template 
+  struct strct;
+};
+#endif
Index: test/Modules/Inputs/nested-template-default-arg-redecl/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/nested-template-default-arg-redecl/module.modulemap
@@ -0,0 +1,24 @@
+module ALIAS1 {
+  header "alias1.h"
+  module ALIAS2 {
+header "alias2.h"
+  }
+}
+module VAR1 {
+  header "var1.h"
+  module VAR2 {
+header "v

[PATCH] D60892: Modules: Search for a visible definition of the decl context when computing visibility of a default template parameter

[David Blaikie via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358795: Modules: Search for a visible definition of the decl 
context when computing… (authored by dblaikie, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60892?vs=195825&id=195931#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60892

Files:
  cfe/trunk/lib/Sema/SemaLookup.cpp
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/alias.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/alias1.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/alias2.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/func.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/func1.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/func2.h
  
cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/module.modulemap
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/strct.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/strct1.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/strct2.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/var.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/var1.h
  cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/var2.h
  cfe/trunk/test/Modules/nested-template-default-arg-redecl.cpp

Index: cfe/trunk/lib/Sema/SemaLookup.cpp
===
--- cfe/trunk/lib/Sema/SemaLookup.cpp
+++ cfe/trunk/lib/Sema/SemaLookup.cpp
@@ -1543,8 +1543,21 @@
 // and in C we must not because each declaration of a function gets its own
 // set of declarations for tags in prototype scope.
 bool VisibleWithinParent;
-if (D->isTemplateParameter() || isa(D) ||
-(isa(DC) && !SemaRef.getLangOpts().CPlusPlus))
+if (D->isTemplateParameter()) {
+  bool SearchDefinitions = true;
+  if (const auto *DCD = dyn_cast(DC)) {
+if (const auto *TD = DCD->getDescribedTemplate()) {
+  TemplateParameterList *TPL = TD->getTemplateParameters();
+  auto Index = getDepthAndIndex(D).second;
+  SearchDefinitions = Index >= TPL->size() || TPL->getParam(Index) != D;
+}
+  }
+  if (SearchDefinitions)
+VisibleWithinParent = SemaRef.hasVisibleDefinition(cast(DC));
+  else
+VisibleWithinParent = isVisible(SemaRef, cast(DC));
+} else if (isa(D) ||
+   (isa(DC) && !SemaRef.getLangOpts().CPlusPlus))
   VisibleWithinParent = isVisible(SemaRef, cast(DC));
 else if (D->isModulePrivate()) {
   // A module-private declaration is only visible if an enclosing lexical
Index: cfe/trunk/test/Modules/nested-template-default-arg-redecl.cpp
===
--- cfe/trunk/test/Modules/nested-template-default-arg-redecl.cpp
+++ cfe/trunk/test/Modules/nested-template-default-arg-redecl.cpp
@@ -0,0 +1,18 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x c++ -fmodules -fimplicit-module-maps -fmodules-cache-path=%t \
+// RUN: -I %S/Inputs/nested-template-default-arg-redecl -std=c++14 \
+// RUN: -fmodules-local-submodule-visibility -verify %s
+#include "alias2.h"
+#include "var2.h"
+#include "strct2.h"
+#include "func2.h"
+
+// FIXME: Variable templates lexical decl context appears to be the translation
+// unit, which is incorrect. Fixing this will hopefully address the following
+// error/bug:
+
+// expected-note@Inputs/nested-template-default-arg-redecl/var.h:4 {{default argument declared here}}
+auto var = &var_outer::var<>; // expected-error {{default argument of 'var' must be imported from module 'VAR1' before it is required}}
+auto func = &func_outer::func<>;
+strct_outer::strct<> *strct;
+alias_outer::alias<> *alias;
Index: cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/strct2.h
===
--- cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/strct2.h
+++ cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/strct2.h
@@ -0,0 +1 @@
+#include "strct.h"
Index: cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/module.modulemap
===
--- cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/module.modulemap
+++ cfe/trunk/test/Modules/Inputs/nested-template-default-arg-redecl/module.modulemap
@@ -0,0 +1,24 @@
+module ALIAS1 {
+  header "alias1.h"
+  module ALIAS2 {
+header "alias2.h"
+  }
+}
+module VAR1 {
+  header "var1.h"
+  module VAR2 {
+header "var2.h"
+  }
+}
+module FUNC1 {
+  header "func1.h"
+  module FUNC2 {
+header "func2.h"
+  }
+}
+module STRCT1 {
+  header "str

[PATCH] D61079: Skip type units/type uniquing when we know we're only emitting the type once (vtable-based emission when triggered by a strong vtable, with -fno-standalone-debug)

[David Blaikie via Phabricator via cfe-commits]

dblaikie created this revision.
dblaikie added a reviewer: aprantl.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

(this would regress size without a corresponding LLVM change that avoids
putting other user defined types inside type units when they aren't in
their own type units - instead emitting declarations inside the type
unit and a definition in the primary CU)

Adrian - can you tell me whether the changes to the implicit modular debug info 
tests are OK? What does that feature use the 'identifier' for, if anything?


Repository:
  rC Clang

https://reviews.llvm.org/D61079

Files:
  lib/CodeGen/CGDebugInfo.cpp
  test/Modules/ExtDebugInfo.cpp
  test/Modules/ModuleDebugInfo.cpp


Index: test/Modules/ModuleDebugInfo.cpp
===
--- test/Modules/ModuleDebugInfo.cpp
+++ test/Modules/ModuleDebugInfo.cpp
@@ -119,8 +119,7 @@
 
 // CHECK: ![[A:.*]] = {{.*}}!DICompositeType(tag: DW_TAG_class_type, name: "A",
 // CHECK-SAME:   elements:
-// CHECK-SAME:   vtableHolder: ![[A]],
-// CHECK-SAME:   identifier: "_ZTS1A")
+// CHECK-SAME:   vtableHolder: ![[A]])
 
 // CHECK: ![[DERIVED:.*]] = {{.*}}!DICompositeType(tag: DW_TAG_class_type, 
name: "Derived",
 // CHECK-SAME: identifier: "_ZTS7Derived")
Index: test/Modules/ExtDebugInfo.cpp
===
--- test/Modules/ExtDebugInfo.cpp
+++ test/Modules/ExtDebugInfo.cpp
@@ -214,7 +214,7 @@
 // CHECK-PCH:dwoId: 18446744073709551614
 
 // CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A",
-// CHECK-SAME: DIFlagFwdDecl, identifier: "_ZTS1A")
+// CHECK-SAME: DIFlagFwdDecl)
 
 // There is a full definition of the type available in the module.
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual",
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -915,6 +915,11 @@
 
   if (!needsTypeIdentifier(TD, CGM, TheCU))
 return Identifier;
+  if (const auto *RD = dyn_cast(TD))
+if (RD->getDefinition())
+  if (RD->isDynamicClass() &&
+  CGM.getVTableLinkage(RD) == llvm::GlobalValue::ExternalLinkage)
+return Identifier;
 
   // TODO: This is using the RTTI name. Is there a better way to get
   // a unique string for a type?


Index: test/Modules/ModuleDebugInfo.cpp
===
--- test/Modules/ModuleDebugInfo.cpp
+++ test/Modules/ModuleDebugInfo.cpp
@@ -119,8 +119,7 @@
 
 // CHECK: ![[A:.*]] = {{.*}}!DICompositeType(tag: DW_TAG_class_type, name: "A",
 // CHECK-SAME:   elements:
-// CHECK-SAME:   vtableHolder: ![[A]],
-// CHECK-SAME:   identifier: "_ZTS1A")
+// CHECK-SAME:   vtableHolder: ![[A]])
 
 // CHECK: ![[DERIVED:.*]] = {{.*}}!DICompositeType(tag: DW_TAG_class_type, name: "Derived",
 // CHECK-SAME: identifier: "_ZTS7Derived")
Index: test/Modules/ExtDebugInfo.cpp
===
--- test/Modules/ExtDebugInfo.cpp
+++ test/Modules/ExtDebugInfo.cpp
@@ -214,7 +214,7 @@
 // CHECK-PCH:dwoId: 18446744073709551614
 
 // CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A",
-// CHECK-SAME: DIFlagFwdDecl, identifier: "_ZTS1A")
+// CHECK-SAME: DIFlagFwdDecl)
 
 // There is a full definition of the type available in the module.
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual",
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -915,6 +915,11 @@
 
   if (!needsTypeIdentifier(TD, CGM, TheCU))
 return Identifier;
+  if (const auto *RD = dyn_cast(TD))
+if (RD->getDefinition())
+  if (RD->isDynamicClass() &&
+  CGM.getVTableLinkage(RD) == llvm::GlobalValue::ExternalLinkage)
+return Identifier;
 
   // TODO: This is using the RTTI name. Is there a better way to get
   // a unique string for a type?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits