Re: [PATCH] D11743: [CMake] First pass at adding support for clang bootstrap builds to CMake
chandlerc added a comment. Other thoughts: Can we get 'bootstrap-check'? 'bootstrap-ccheck-foo' for all the 'check-foo' variants? I think 'bootstrap' shouldn't do 'bootstrap-install', it should be equivalent to 'bootstrap-check-all' IMO. http://reviews.llvm.org/D11743 ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] D11743: [CMake] First pass at adding support for clang bootstrap builds to CMake
chandlerc added a comment. This is pretty awesome. A question -- how can we document better what this does? In particular, the fact that we have both 'bootstrap-install' and 'install' seems... confusing. Is it possible to make 'install' == 'bootstrap-install' in the case where the CMake option is set? http://reviews.llvm.org/D11743 ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r243945 - [UB] Fix two cases of UB in copy/pasted code from SmallVector.
Author: chandlerc
Date: Mon Aug 3 22:52:52 2015
New Revision: 243945
URL: http://llvm.org/viewvc/llvm-project?rev=243945view=rev
Log:
[UB] Fix two cases of UB in copy/pasted code from SmallVector.
We should really stop copying and pasting code around. =/
Found by UBSan.
Modified:
cfe/trunk/include/clang/AST/ASTVector.h
cfe/trunk/include/clang/Analysis/Support/BumpVector.h
Modified: cfe/trunk/include/clang/AST/ASTVector.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTVector.h?rev=243945r1=243944r2=243945view=diff
==
--- cfe/trunk/include/clang/AST/ASTVector.h (original)
+++ cfe/trunk/include/clang/AST/ASTVector.h Mon Aug 3 22:52:52 2015
@@ -384,14 +384,15 @@ void ASTVectorT::grow(const ASTContext
T *NewElts = new (C, llvm::alignOfT()) T[NewCapacity];
// Copy the elements over.
- if (std::is_classT::value) {
-std::uninitialized_copy(Begin, End, NewElts);
-// Destroy the original elements.
-destroy_range(Begin, End);
- }
- else {
-// Use memcpy for PODs (std::uninitialized_copy optimizes to memmove).
-memcpy(NewElts, Begin, CurSize * sizeof(T));
+ if (Begin != End) {
+if (std::is_classT::value) {
+ std::uninitialized_copy(Begin, End, NewElts);
+ // Destroy the original elements.
+ destroy_range(Begin, End);
+} else {
+ // Use memcpy for PODs (std::uninitialized_copy optimizes to memmove).
+ memcpy(NewElts, Begin, CurSize * sizeof(T));
+}
}
// ASTContext never frees any memory.
Modified: cfe/trunk/include/clang/Analysis/Support/BumpVector.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Support/BumpVector.h?rev=243945r1=243944r2=243945view=diff
==
--- cfe/trunk/include/clang/Analysis/Support/BumpVector.h (original)
+++ cfe/trunk/include/clang/Analysis/Support/BumpVector.h Mon Aug 3 22:52:52
2015
@@ -223,14 +223,15 @@ void BumpVectorT::grow(BumpVectorConte
T *NewElts = C.getAllocator().template AllocateT(NewCapacity);
// Copy the elements over.
- if (std::is_classT::value) {
-std::uninitialized_copy(Begin, End, NewElts);
-// Destroy the original elements.
-destroy_range(Begin, End);
- }
- else {
-// Use memcpy for PODs (std::uninitialized_copy optimizes to memmove).
-memcpy(NewElts, Begin, CurSize * sizeof(T));
+ if (Begin != End) {
+if (std::is_classT::value) {
+ std::uninitialized_copy(Begin, End, NewElts);
+ // Destroy the original elements.
+ destroy_range(Begin, End);
+} else {
+ // Use memcpy for PODs (std::uninitialized_copy optimizes to memmove).
+ memcpy(NewElts, Begin, CurSize * sizeof(T));
+}
}
// For now, leak 'Begin'. We can add it back to a freelist in
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r243946 - [UB] Fix the two ways that we would try to memcpy from a null buffer in
Author: chandlerc
Date: Mon Aug 3 22:52:56 2015
New Revision: 243946
URL: http://llvm.org/viewvc/llvm-project?rev=243946view=rev
Log:
[UB] Fix the two ways that we would try to memcpy from a null buffer in
the nested name specifier code.
First, skip the entire thing when the input is empty.
Next, handle the case where we started off with a null buffer and a zero
capacity to skip copying and freeing.
This was found with UBSan.
Modified:
cfe/trunk/lib/AST/NestedNameSpecifier.cpp
Modified: cfe/trunk/lib/AST/NestedNameSpecifier.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/NestedNameSpecifier.cpp?rev=243946r1=243945r2=243946view=diff
==
--- cfe/trunk/lib/AST/NestedNameSpecifier.cpp (original)
+++ cfe/trunk/lib/AST/NestedNameSpecifier.cpp Mon Aug 3 22:52:56 2015
@@ -435,17 +435,19 @@ TypeLoc NestedNameSpecifierLoc::getTypeL
namespace {
void Append(char *Start, char *End, char *Buffer, unsigned BufferSize,
unsigned BufferCapacity) {
+if (Start == End)
+ return;
+
if (BufferSize + (End - Start) BufferCapacity) {
// Reallocate the buffer.
- unsigned NewCapacity
- = std::max((unsigned)(BufferCapacity? BufferCapacity * 2
-: sizeof(void*) * 2),
- (unsigned)(BufferSize + (End - Start)));
+ unsigned NewCapacity = std::max(
+ (unsigned)(BufferCapacity ? BufferCapacity * 2 : sizeof(void *) * 2),
+ (unsigned)(BufferSize + (End - Start)));
char *NewBuffer = static_castchar *(malloc(NewCapacity));
- memcpy(NewBuffer, Buffer, BufferSize);
-
- if (BufferCapacity)
+ if (BufferCapacity) {
+memcpy(NewBuffer, Buffer, BufferSize);
free(Buffer);
+ }
Buffer = NewBuffer;
BufferCapacity = NewCapacity;
}
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r243947 - [UB] When attaching empty strings to the AST, use an empty StringRef
Author: chandlerc
Date: Mon Aug 3 22:52:58 2015
New Revision: 243947
URL: http://llvm.org/viewvc/llvm-project?rev=243947view=rev
Log:
[UB] When attaching empty strings to the AST, use an empty StringRef
rather than forcing the bump pointer allocator to produce a viable
pointer. This also fixes UB when we would try to memcpy from the null
incoming StringRef.
Modified:
cfe/trunk/lib/AST/Stmt.cpp
Modified: cfe/trunk/lib/AST/Stmt.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Stmt.cpp?rev=243947r1=243946r2=243947view=diff
==
--- cfe/trunk/lib/AST/Stmt.cpp (original)
+++ cfe/trunk/lib/AST/Stmt.cpp Mon Aug 3 22:52:58 2015
@@ -724,6 +724,8 @@ MSAsmStmt::MSAsmStmt(const ASTContext C
}
static StringRef copyIntoContext(const ASTContext C, StringRef str) {
+ if (str.empty())
+return StringRef();
size_t size = str.size();
char *buffer = new (C) char[size];
memcpy(buffer, str.data(), size);
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r243950 - [UB] Avoid a really broken call to realloc that would later result in
Author: chandlerc
Date: Mon Aug 3 22:53:04 2015
New Revision: 243950
URL: http://llvm.org/viewvc/llvm-project?rev=243950view=rev
Log:
[UB] Avoid a really broken call to realloc that would later result in
a bad call to memcpy.
When we only have a buffer from one of the two reparse calls, we can
just return that buffer rather than going through the realloc/memcpy
dance.
Found with UBsan.
Modified:
cfe/trunk/tools/c-index-test/c-index-test.c
Modified: cfe/trunk/tools/c-index-test/c-index-test.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/c-index-test/c-index-test.c?rev=243950r1=243949r2=243950view=diff
==
--- cfe/trunk/tools/c-index-test/c-index-test.c (original)
+++ cfe/trunk/tools/c-index-test/c-index-test.c Mon Aug 3 22:53:04 2015
@@ -255,6 +255,17 @@ static int parse_remapped_files_with_try
if (ret)
return ret;
+ if (num_unsaved_files_no_try_idx == 0) {
+*unsaved_files = unsaved_files_try_idx;
+*num_unsaved_files = num_unsaved_files_try_idx;
+return 0;
+ }
+ if (num_unsaved_files_try_idx == 0) {
+*unsaved_files = unsaved_files_no_try_idx;
+*num_unsaved_files = num_unsaved_files_no_try_idx;
+return 0;
+ }
+
*num_unsaved_files = num_unsaved_files_no_try_idx +
num_unsaved_files_try_idx;
*unsaved_files
= (struct CXUnsavedFile *)realloc(unsaved_files_no_try_idx,
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r243949 - [UB] Guard two calls to memcpy in generated attribute code to handle
Author: chandlerc
Date: Mon Aug 3 22:53:01 2015
New Revision: 243949
URL: http://llvm.org/viewvc/llvm-project?rev=243949view=rev
Log:
[UB] Guard two calls to memcpy in generated attribute code to handle
null StringRef objects as inputs.
Found by UBSan.
Modified:
cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
Modified: cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp?rev=243949r1=243948r2=243949view=diff
==
--- cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp (original)
+++ cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp Mon Aug 3 22:53:01 2015
@@ -326,7 +326,8 @@ namespace {
OSgetLowerName() Length = S.size();\n;
OS this- getLowerName() = new (C, 1) char [
getLowerName() Length];\n;
- OS std::memcpy(this- getLowerName() , S.data(),
+ OS if (!S.empty())\n;
+ OSstd::memcpy(this- getLowerName() , S.data(),
getLowerName() Length);\n;
OS};
}
@@ -337,7 +338,8 @@ namespace {
OS A-get getUpperName() ();
}
void writeCtorBody(raw_ostream OS) const override {
- OSstd::memcpy( getLowerName() , getUpperName()
+ OSif (! getUpperName() .empty())\n;
+ OS std::memcpy( getLowerName() , getUpperName()
.data(), getLowerName() Length);;
}
void writeCtorInitializers(raw_ostream OS) const override {
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r243948 - [UB] Another place where we were trying to put string data into
Author: chandlerc
Date: Mon Aug 3 22:53:00 2015
New Revision: 243948
URL: http://llvm.org/viewvc/llvm-project?rev=243948view=rev
Log:
[UB] Another place where we were trying to put string data into
a BumpPtrAllocator. This at least now handles the case where there is no
concatentation without calling memcpy on a null pointer. It might be
interesting to handle the case where everything is empty without
round-tripping through the allocator, but it wasn't clear to me if the
pointer returned is significant in any way, so I've left it in
a conservatively more-correct state.
Again, found with UBSan.
Modified:
cfe/trunk/lib/CodeGen/CGDebugInfo.h
Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.h?rev=243948r1=243947r2=243948view=diff
==
--- cfe/trunk/lib/CodeGen/CGDebugInfo.h (original)
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.h Mon Aug 3 22:53:00 2015
@@ -485,8 +485,10 @@ private:
/// are concatenated.
StringRef internString(StringRef A, StringRef B = StringRef()) {
char *Data = DebugInfoNames.Allocatechar(A.size() + B.size());
-std::memcpy(Data, A.data(), A.size());
-std::memcpy(Data + A.size(), B.data(), B.size());
+if (!A.empty())
+ std::memcpy(Data, A.data(), A.size());
+if (!B.empty())
+ std::memcpy(Data + A.size(), B.data(), B.size());
return StringRef(Data, A.size() + B.size());
}
};
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] D11599: Change some series of AddAllArgs calls to use the most general AddAlllArgs.
chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Very much prefer preserving the original order. We were already working around the reorderings by putting the D and U groups into a single call. =/ This approach seems *much* cleaner. http://reviews.llvm.org/D11599 ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] D11581: [SHAVE] Pass all -f, -g, -O, -W options through directly to moviCompile.
chandlerc added a subscriber: chandlerc.
chandlerc added a comment.
Comments from the peanut gallery...
Comment at: lib/Driver/Tools.cpp:9482-9488
@@ -9481,13 +9481,9 @@
- // Any -O option passes through without translation. What about -Ofast ?
- if (Arg *A = Args.getLastArg(options::OPT_O_Group))
-A-render(Args, CmdArgs);
-
- if (Args.hasFlag(options::OPT_ffunction_sections,
- options::OPT_fno_function_sections)) {
-CmdArgs.push_back(-ffunction-sections);
- }
- if (Args.hasArg(options::OPT_fno_inline_functions))
-CmdArgs.push_back(-fno-inline-functions);
-
+ Args.AddAllArgs(CmdArgs,
+ options::OPT_f_Group,
+ options::OPT_f_clang_Group,
+ options::OPT_g_Group);
+ Args.AddAllArgs(CmdArgs,
+ options::OPT_O_Group,
+ options::OPT_W_Group);
CmdArgs.push_back(-fno-exceptions); // Always do this even if unspecified.
How worried are you about eventual incompatibilities between the flags in these
groups for the driver and for the underlying SHAVE compiler? I'm moderately
worried...
How many flags are we talking about here? Also, the -W flags worry me less than
the rest.
Finally, why two AddAllArgs?
http://reviews.llvm.org/D11581
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [libunwind] Proposal to merge patches for libc++abi to 3.7
FWIW, I agree, this seems really important. On Sun, Jul 26, 2015 at 8:54 AM Logan Chien tzuhsiang.ch...@gmail.com wrote: Hi Hans and Nick, I would like to propose to merge following patches to libunwind 3.7 branch: r242642: libunwind: Introduce __libunwind_config.h. r243073: unwind: Fix libc++abi and libgcc build. We need these changes to build libc++abi with libgcc (without libunwind.so.) IMO, this is an important fix that should be merged to 3.7 release. May you have a look? Thanks for your help! Sincerely, Logan ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] D11346: Extend misc-unused-parameters to delete parameters of static functions
chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Feel free to address the comments below in a follow-up patch. This seems a fine next step, LGTM Comment at: clang-tidy/misc/UnusedParametersCheck.cpp:76 @@ +75,3 @@ + + // Handle local functions by deleting the parameters. + unsigned ParamIndex = Param-getFunctionScopeIndex(); Hmm, I thought of a reason to not delete parameters from local functions -- if they're used in some way other than calling them such as using them as an argument to a template. Maybe check for non-CallExpr DeclRefExprs of the function, and fall back to the commenting strategy? Comment at: test/clang-tidy/misc-unused-parameters.cpp:23-24 @@ +22,4 @@ + +// Remove parameters of local functions +// +static void staticFunctionA(int i); Does this already handle function in anonymous namespaces? Want to add those as a follow-up? The next obvious chunk is to handle non-virtual methods of classes in anonymous namespaces. Perhaps those will be handled automatically though. http://reviews.llvm.org/D11346 ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] D11322: Silence the driver warnings, if we see -w on the Driver.
chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM http://reviews.llvm.org/D11322 ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r241620 - Wrap clang modules and pch files in an object file container.
On Mon, Jul 13, 2015 at 7:27 PM Richard Smith rich...@metafoo.co.uk wrote: On Mon, Jul 13, 2015 at 6:02 PM, Adrian Prantl apra...@apple.com wrote: On Jul 13, 2015, at 5:47 PM, Richard Smith rich...@metafoo.co.uk wrote: On Mon, Jul 13, 2015 at 3:06 PM, Adrian Prantl apra...@apple.com wrote: On Jul 13, 2015, at 2:00 PM, Eric Christopher echri...@gmail.com wrote: Hi Adrian, Finally getting around to looking at some of this and I think it's going in slightly the wrong direction. In general I think begin -able- to put modules in object files to simplify wrapping, use, etc is a good thing. I think being required to do so is somewhat problematic. Let me start with that the current infrastructure already allows selecting whether you want wrapped modules or not by passing the appropriate PCHContainerOperations object to CompilerInstance. Clang currently unconditionally uses an object file wrapper, all of clang-tools-extra doesn’t. We could easily control the behavior of clang based on a (new) command line option. But.. on a platform with a shared module cache you always have to assume that a module once built will eventually be used by a client that wants to read the debug info. Think llvm-dsymutil — it does not know and does not want to know how to build clang modules, but does want to read all the debug info from a clang module. Imagine, for example, you have a giant distributed build system... You'd want to create a pile of modules (that may reference/include/etc other modules) that aren't don't or may not have debug information as part of them (because you might want to build without it or have the debug info alongside it as a separate compilation). Waiting on the full build of the module including debug is going to adversely affect your overall build time and so shouldn't be necessary - especially if you want to be able to have information separate ultimately. Make sense? Not sure if you would be saving much by having the debug info separately, from what I’ve measured so far the debug info for a module makes up less than 10% of the total size. Admittedly, build-time-wise going through the backend to emit the object file is a lot more expensive than just dumping the raw PCH. [1] Yeah, I think wanting to be able to control the behavior is reasonable, we just need to be careful what the implications for consumers are. If we add a, e.g., an “-fraw-modules” [2] or switch to clang to turn off the object file wrapping, I’d strongly suggest that we add the value of this switch to the module hash (or add a an optional “-g” to the module file name after the hash or something like that) to avoid ugly race conditions between debug info and non-debug-info builds of the same module. This way we’d have essentially two separate module caches, with and without debug info. That's fine, I think (we don't use a module cache at all in our build system; it doesn't really make much sense for a distributed build) and most command-line flag changes already have this effect. Great! would that work for you? -- adrian [1] If you want to be serious about building the module debug info in parallel to the rest of the build, you could even have a clang-based tool import the just-built raw clang module and emit the debug info without having to parse the headers again :-) That is what we intend to do :) (Assuming this turns out to actually be faster than re-parsing; faulting in the entire contents of a module has much worse locality than parsing.) [2] -fraw-modules, -fmodule-format-raw, -fmodule-debug-info, ...? I would imagine that the driver enables module debug info when -gmodules” is present and by default on Darwin. That seems reasonable to me. For the frontend flag, I think a flag to turn this on or to select the module format makes more sense than a flag to switch to the raw format. Okay then let’s narrow this down. Other possibilities in that direction include (sorted from subjectively best to worst) -fmodule-format=obj -fmodule-debug-info -ffat-modules -fmodule-container -fmodule-container-object It's a -cc1 flag, so it doesn't really matter much. But eventually we need a reasonable way to support non-implicit-cache usage of modules without passing -cc1 flags. For some build systems and environments, I actually suspect that non-implicit-cache builds will be the default. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [LLVMdev] Inline hint for methods defined in-class
On Tue, Jul 7, 2015 at 10:27 PM Xinliang David Li davi...@google.com wrote: On Tue, Jul 7, 2015 at 6:06 PM, Chandler Carruth chandl...@gmail.com wrote: On Tue, Jul 7, 2015 at 4:11 PM Easwaran Raman era...@google.com wrote: I'm reviving this thread after a while and CCing cfe-commits as suggested by David Blaikie. I've also collected numbers building chrome (from chromium, on Linux) with and without this patch as suggested by David. I've re-posted the proposed patch and performance/size numbers collected at the top to make it easily readable for those reading it through cfe-commits. First off, thanks for collecting the numbers and broadening the distribution. Also, sorry it took me so long to get over to this thread. I want to lay out my stance on this issue at a theoretical and practical level first. I'll follow up with thoughts on the numbers as well after that. I think that tying *any* optimizer behavior to the 'inline' keyword is fundamentally the wrong direction. Chandler, thanks for sharing your thought -- however I don't think it is wrong, let alone 'fundamentally wrong'. Despite all the analysis that can be done, the inliner is in the end heuristic based. In lack of the profile data, when inlining two calls yield the same static benefit and size cost, it is reasonable for the inliner to think the call to the function with inline hint to yield more high dynamic/runtime benefit -- thus it has a higher static size budget to burn. This is an argument for having *some* source level hinting construct. While I think such a construct is very risky, I did actually suggest that we add such a hint because I recognize some of the practical necessities for it. My primary argument is against using the 'inline' keyword as the source of the hint, and especially using the happenstance of a method body in a class as the source of the hint. We have reasons why we have done this historically, and we can't just do an immediate about face, but we should be actively looking for ways to *reduce* the optimizer's reliance on this keyword to convey any meaning whatsoever. yes those additional things will be done, but they are really orthogonal. I'm not talking about additional things, I'm talking about separating the optimization hint from the semantics and linkage changing constructs. That does not seem orthogonal. The reason I think that is the correct direction is because, for better or worse, the 'inline' keyword in C++ is not about optimization, but about linkage. It is about both optimization and linkage. In fact the linkage simply serves as an implementation detail. In C++ standard 7.1.2, paragraph 2 says: A function declaration (8.3.5, 9.3, 11.3) with an inline specifier declares an inline function. The inline specifier indicates to the implementation that inline substitution of the function body at the point of call is to be preferred to the usual function call mechanism. An implementation is not required to perform this inline substitution at the point of call; however, even if this inline substitution is omitted, the other rules for inline functions defined by 7.1.2 shall still be respected. Developers see those and rely on those to give compiler the hints. This is essentially a nonsense paragraph for a standardized specification of a programming language. How hard you optimize code doesn't have any bearing on the conformance and behavior of the program. =/ I think this paragraph is part of the historical context. I think we should change C++ to remove it (and I will propose that change) and I think we should change C++ to support a standardized *non*-semantic hint if folks really want to see that in the form of a C++11-style [[attribute]]. I'm also really, really confident that most developers are not using the wording of the standard as the basis of how they tune the performance of their code. =/ Most importantly, paragraph 3 says: A function defined within a class definition is an inline function. The inline specifier shall not appear on a block scope function declaration.93 If the inline specifier is used in a friend declaration, that declaration shall be a definition or the function shall have previously been declared inline. Here we can see regardless of how optimizer will honor the hint and to what extent, and based on what analysis, it is basically incorrect to drop the attribute on the floor for in-class function definitions. Eswaran's fix is justified with this reason alone. The side effect of changing inliner behavior is irrelevant. I don't really understand what you're saying. Clang correctly carries all of the *semantics* required for in-class method bodies. We simply don't attach an optimization hint? I don't think this is incorrect. Nothing in the standard says how hard we should try (and it can't, which is why the standard doesn't make sense to give advice here). It has
Re: [LLVMdev] Inline hint for methods defined in-class
On Wed, Jul 8, 2015 at 1:46 PM Hal Finkel hfin...@anl.gov wrote: - Original Message - From: Xinliang David Li davi...@google.com To: Chandler Carruth chandl...@gmail.com Cc: cfe-commits@cs.uiuc.edu, llvm...@cs.uiuc.edu List llvm...@cs.uiuc.edu Sent: Wednesday, July 8, 2015 12:25:18 AM Subject: Re: [LLVMdev] Inline hint for methods defined in-class On Tue, Jul 7, 2015 at 6:06 PM, Chandler Carruth chandl...@gmail.com wrote: On Tue, Jul 7, 2015 at 4:11 PM Easwaran Raman era...@google.com wrote: I'm reviving this thread after a while and CCing cfe-commits as suggested by David Blaikie. I've also collected numbers building chrome (from chromium, on Linux) with and without this patch as suggested by David. I've re-posted the proposed patch and performance/size numbers collected at the top to make it easily readable for those reading it through cfe-commits. First off, thanks for collecting the numbers and broadening the distribution. Also, sorry it took me so long to get over to this thread. I want to lay out my stance on this issue at a theoretical and practical level first. I'll follow up with thoughts on the numbers as well after that. I think that tying *any* optimizer behavior to the 'inline' keyword is fundamentally the wrong direction. Chandler, thanks for sharing your thought -- however I don't think it is wrong, let alone 'fundamentally wrong'. Despite all the analysis that can be done, the inliner is in the end heuristic based. In lack of the profile data, when inlining two calls yield the same static benefit and size cost, it is reasonable for the inliner to think the call to the function with inline hint to yield more high dynamic/runtime benefit -- thus it has a higher static size budget to burn. We have reasons why we have done this historically, and we can't just do an immediate about face, but we should be actively looking for ways to *reduce* the optimizer's reliance on this keyword to convey any meaning whatsoever. yes those additional things will be done, but they are really orthogonal. The reason I think that is the correct direction is because, for better or worse, the 'inline' keyword in C++ is not about optimization, but about linkage. It is about both optimization and linkage. In fact the linkage simply serves as an implementation detail. In C++ standard 7.1.2, paragraph 2 says: The fact that C++ combines, into one keyword, a change in semantics (linkage) and an optimization hint is quite unfortunate. I wish it were otherwise. We could work to change it? I specifically proposed adding a way to move away from this unfortunate design. However, as it stands, I support this change. The benchmark numbers are encouraging, and it replaces an implementation quirk with the underlying (unfortunate) language design choice. The implementation quirk is that putting the inline keyword on an in-class function definition changes the behavior of the optimizer. However, according to the language specification, that definition should have implied that keyword. While an implementation is certainly free to do arbitrary things with hints, this behavior violates the spirit of the language specification. I strongly disagree that this is the spirit of the language specification. Even if it was historically, I think we should move away from that. The language shouldn't be trying to do this with a language keyword, and it shouldn't be coupling semantics to hints. I'm very happy to take this up with the committee, but I don't see why we shouldn't push Clang in that direction here when there is no issue of conformance. To see how broken this is, let's look at how miserably small the difference is between the un-hinted and hinted thresholds. We've ended up shrinking this difference over time in LLVM because increasing the hinted threshold caused lots of performance regressions and size regressions. It makes a meaningless use of a standardized keyword meaningful, and that's the greater transgression. So here is what I want to do: 1) Add a non-semantic attribute that conveys this hint. We could even convey a much *stronger* hint with this rather than just a tiny hint the way it is today because it wouldn't end up being forced onto every template regardless of whether that makes sense. 2) Start lobbying to remove the hint from the 'inline' keyword by working with the people who see regressions from this to use the new annotation to recover the performance. 3) Completely remove the semantic coupling of the optimizer hint and fix the meaningless use of the standardized keyword at the same time. But the more places where we use the inline hint today, the harder #2 will become. I've already tried once before to remove the hint and couldn't because of benchmarks that had been tightly tuned and coupled to the existing (quirky
Re: [LLVMdev] Inline hint for methods defined in-class
On Tue, Jul 7, 2015 at 4:11 PM Easwaran Raman era...@google.com wrote:
I'm reviving this thread after a while and CCing cfe-commits as
suggested by David Blaikie. I've also collected numbers building
chrome (from chromium, on Linux) with and without this patch as
suggested by David. I've re-posted the proposed patch and
performance/size numbers collected at the top to make it easily
readable for those reading it through cfe-commits.
First off, thanks for collecting the numbers and broadening the
distribution. Also, sorry it took me so long to get over to this thread.
I want to lay out my stance on this issue at a theoretical and practical
level first. I'll follow up with thoughts on the numbers as well after that.
I think that tying *any* optimizer behavior to the 'inline' keyword is
fundamentally the wrong direction. We have reasons why we have done this
historically, and we can't just do an immediate about face, but we should
be actively looking for ways to *reduce* the optimizer's reliance on this
keyword to convey any meaning whatsoever.
The reason I think that is the correct direction is because, for better or
worse, the 'inline' keyword in C++ is not about optimization, but about
linkage. It has a functional impact and can be both necessary or impossible
to use to meet those functional requirements. This in turn leaves
programmers in a lurch if the functional requirements are ever in tension
with the optimizer requirements.
We're also working really hard to get more widely deployed cross-module
optimization strategies, in part to free programmers from the requirement
that they put all their performance critical code in header files. That
makes compilation faster, and has lots of benefits to the factoring and
design of the code itself. We shouldn't then create an incentive to keep
things in header files so that they pick up a hint to the optimizer.
Ultimately, the world will be a better place if we can eventually move code
away from relying on the hint provided by the 'inline' keyword to the
optimizer.
That doesn't mean that the core concept of hinting to the optimizer that a
particular function is a particularly good candidate for inlining is
without value. While I think it is a bad practice that we shouldn't
encourage in code (especially portable code) I can see the desire to at
least have *some* attribute which is nothing more or less than a hint to
the optimizer to inline harder[1]. It would help people work around inliner
bugs in the short term, and even help debug inliner-rooted optimization
problems. Codebases with strong portability requirements could still (and
probably should) forbid or tightly control access to this kind of hint. I
would want really strong documentation about how this attribute *completely
voids* your performance warranty (if such a thing exists) as from version
to version of the compiler it may go from a helpful hint to a devastatingly
bad hint. But I think I could be persuaded to live with such a hint
existing. But I'm *really* uncomfortable with it being tied to something
that also impacts linkage or other semantics of the program.
[1]: Currently, the only other hint we have available is pretty terrible as
it *also* has semantic effects: the always_inline attribute.
The proposed patch will add InlineHint to methods defined inside a class:
--- a/lib/CodeGen/CodeGenFunction.cpp
+++ b/lib/CodeGen/CodeGenFunction.cpp
@@ -630,7 +630,7 @@ void CodeGenFunction::StartFunction(GlobalDecl GD,
if (const FunctionDecl *FD = dyn_cast_or_nullFunctionDecl(D)) {
if (!CGM.getCodeGenOpts().NoInline) {
for (auto RI : FD-redecls())
-if (RI-isInlineSpecified()) {
+if (RI-isInlined()) {
Fn-addFnAttr(llvm::Attribute::InlineHint);
break;
}
Here are the performance and size numbers I've collected:
- C++ subset of Spec: No performance effects, 0.1% size increase
(all size numbers are text sizes returned by 'size')
- Clang: 0.9% performance improvement (both -O0 and -O2 on a large .ii
file) , 4.1% size increase
FWIW, this size increase seems *really* bad. I think that kills this
approach even in the short term.
- Chrome: no performance improvement, 0.24% size increase
- Google internal benchmark suite (geomean of ~20 benchmarks): ~1.8%
performance improvement, no size regression
I'm also somewhat worried about the lack of any performance improvements
outside of the Google benchmarks. That somewhat strongly suggests that our
benchmarks are overly coupled to this hint already. The fact that neither
Chrome, Clang, nor SPEC improved is... not at all encouraging.
-Chandler
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] clang-format some of the files in lib/Driver. NFC
LGTM with a trivial comment tweak below.
Comment at: lib/Driver/Driver.cpp:1255-1256
@@ -1257,4 +1254,4 @@
std::unique_ptrAction Current(new InputAction(*InputArg, InputType));
-for (SmallVectorImplphases::ID::iterator
- i = PL.begin(), e = PL.end(); i != e; ++i) {
+for (SmallVectorImplphases::ID::iterator i = PL.begin(), e = PL.end();
+ i != e; ++i) {
phases::ID Phase = *i;
silvas wrote:
Quick drive-by comment: A few of these formatting changes are iterator for
loops that could be range-for'ified. Would be nice to do that cleanup where
possible.
While I agree, I'm happy for Doug to leave that to a later cleanup (or to leave
that cleanup for another). This will at least reduce the (currently
substantial) noise in code reviews from incidental formatting changes.
Comment at: lib/Driver/ToolChains.cpp:3544-3546
@@ -3626,11 +3543,5 @@
const std::string LibStdCXXIncludePathCandidates[] = {
-// Gentoo is weird and places its headers inside the GCC install, so if the
-// first attempt to find the headers fails, try these patterns.
-InstallDir.str() + /include/g++-v + Version.MajorStr + . +
-Version.MinorStr,
-InstallDir.str() + /include/g++-v + Version.MajorStr,
-// Android standalone toolchain has C++ headers in yet another place.
-LibDir.str() + /../ + TripleStr.str() + /include/c++/ + Version.Text,
-// Freescale SDK C++ headers are directly in sysroot/usr/include/c++,
-// without a subdirectory corresponding to the gcc version.
-LibDir.str() + /../include/c++,
+ // Gentoo is weird and places its headers inside the GCC install, so if
+ // the
+ // first attempt to find the headers fails, try these patterns.
+ InstallDir.str() + /include/g++-v + Version.MajorStr + . +
Re-flow this comment block?
Comment at: lib/Driver/Tools.cpp:5507-5508
@@ -5559,3 +5506,4 @@
else
- // Don't render as input, we need gcc to do the translations. FIXME:
Pranav: What is this ?
+ // Don't render as input, we need gcc to do the translations. FIXME:
+ // Pranav: What is this ?
II.getInputArg().render(Args, CmdArgs);
This comment is so bad it blows my mind.
http://reviews.llvm.org/D10689
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Call attention to strange case in getMultiarchTriple, and DRY so much.
I think you should just fix the code. This seems clearly a bug. With that fix, this LGTM. Comment at: lib/Driver/ToolChains.cpp:3096 @@ -3093,2 +3095,3 @@ return powerpc64-linux-gnu; +// Fallthrough case llvm::Triple::ppc64le: This is certainly wrong. Please just break here, that is what the code should have been doing. We don't want to synthesize a LE multiarch location for a non-LE triple ever. (Also, good catch!) http://reviews.llvm.org/D10605 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Mostly standardize on MumbleToolChain as the ToolChain subclass name. NFC
OMG I hate the commandeer thing in Phab. I always think it will make me a reviewr. :: sigh :: I go to try and fix this. http://reviews.llvm.org/D10609 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Mostly standardize on MumbleToolChain as the ToolChain subclass name. NFC
Oh great, I can't. Doug, sorry, just commandeer the revision back. Sorry i broke the review tool... http://reviews.llvm.org/D10609 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Mostly standardize on MumbleToolChain as the ToolChain subclass name. NFC
Please update once this is rebased on top of the formatted variant. I'd like to glance at it then before I stamp it. Thanks for the cleanup!! http://reviews.llvm.org/D10609 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r240530 - Remove a limited and somewhat questionable DenseMapInfo specialization
Author: chandlerc
Date: Wed Jun 24 05:24:30 2015
New Revision: 240530
URL: http://llvm.org/viewvc/llvm-project?rev=240530view=rev
Log:
Remove a limited and somewhat questionable DenseMapInfo specialization
for StringRef now that the core DenseMap library provides this facility.
Modified:
cfe/trunk/lib/Basic/VirtualFileSystem.cpp
Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=240530r1=240529r2=240530view=diff
==
--- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
+++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Wed Jun 24 05:24:30 2015
@@ -324,20 +324,6 @@ directory_iterator OverlayFileSystem::di
// VFSFromYAML implementation
//===---===/
-// Allow DenseMapStringRef, This is useful below because we know all
the
-// strings are literals and will outlive the map, and there is no reason to
-// store them.
-namespace llvm {
- template
- struct DenseMapInfoStringRef {
-// This assumes that will never be a valid key.
-static inline StringRef getEmptyKey() { return StringRef(); }
-static inline StringRef getTombstoneKey() { return StringRef(); }
-static unsigned getHashValue(StringRef Val) { return HashString(Val); }
-static bool isEqual(StringRef LHS, StringRef RHS) { return LHS == RHS; }
- };
-}
-
namespace {
enum EntryKind {
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Pedantically rename all Tool subclasses to be nouns, not verbs. NFC
LGTM, thanks for the cleanup!
Comment at: lib/Driver/Tools.h:196-197
@@ -192,7 +195,4 @@
namespace hexagon {
- // For Hexagon, we do not need to instantiate tools for PreProcess,
PreCompile and Compile.
- // We simply use clang -cc1 for those actions.
- class LLVM_LIBRARY_VISIBILITY Assemble : public GnuTool {
- public:
-Assemble(const ToolChain TC) : GnuTool(hexagon::Assemble,
- hexagon-as, TC) {}
+// For Hexagon, we do not need to instantiate tools for PreProcess, PreCompile
+// and Compile.
+// We simply use clang -cc1 for those actions.
This seems like an unrelated formatting change? I don't really care about it
though.
http://reviews.llvm.org/D10595
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Mostly standardize on MumbleToolChain as the ToolChain subclass name. NFC
The content here seems fine, but clang-format has changed a lot of unrelated lines of code. How about sending me just a fresh clang-format of lib/Driver? You'll want to look at the result and revert some hunks that don't make sense I suspect, but that would give you a much more consistent and clean baseline. http://reviews.llvm.org/D10609 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Pedantically rename all Tool subclasses to be nouns, not verbs. NFC
Comment at: lib/Driver/Tools.h:170
@@ -171,1 +169,3 @@
+Compiler(const ToolChain TC)
+: Common(gcc::Compile, gcc frontend, TC) {}
Above you renamed gcc::Preprocess to gcc::Preprocessor, but here (and
almost everywhere else in my spot check) the text names were left alone.
I'm not actually sure which is right. These seem like documentation or logging
generation strings, so maybe they should all be updated?
http://reviews.llvm.org/D10595
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Remove duplicated alteration to getProgramPaths().
Just to be clear, this commit seems good to me, and I'm fine with it being
in, but please don't submit patches without review once you've asked for it.
On Tue, Jun 16, 2015 at 2:22 PM Chandler Carruth chandl...@google.com
wrote:
Doug, why did you commit this without review? That's not really cool after
you've asked folks to code review it and without any update.
On Tue, Jun 16, 2015 at 12:43 PM Douglas Katzman do...@google.com wrote:
REPOSITORY
rL LLVM
http://reviews.llvm.org/D10444
Files:
cfe/trunk/lib/Driver/ToolChains.cpp
Index: cfe/trunk/lib/Driver/ToolChains.cpp
===
--- cfe/trunk/lib/Driver/ToolChains.cpp
+++ cfe/trunk/lib/Driver/ToolChains.cpp
@@ -42,10 +42,6 @@
MachO::MachO(const Driver D, const llvm::Triple Triple,
const ArgList Args)
: ToolChain(D, Triple, Args) {
- getProgramPaths().push_back(getDriver().getInstalledDir());
- if (getDriver().getInstalledDir() != getDriver().Dir)
-getProgramPaths().push_back(getDriver().Dir);
-
// We expect 'as', 'ld', etc. to be adjacent to our install dir.
getProgramPaths().push_back(getDriver().getInstalledDir());
if (getDriver().getInstalledDir() != getDriver().Dir)
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Remove duplicated alteration to getProgramPaths().
Doug, why did you commit this without review? That's not really cool after
you've asked folks to code review it and without any update.
On Tue, Jun 16, 2015 at 12:43 PM Douglas Katzman do...@google.com wrote:
REPOSITORY
rL LLVM
http://reviews.llvm.org/D10444
Files:
cfe/trunk/lib/Driver/ToolChains.cpp
Index: cfe/trunk/lib/Driver/ToolChains.cpp
===
--- cfe/trunk/lib/Driver/ToolChains.cpp
+++ cfe/trunk/lib/Driver/ToolChains.cpp
@@ -42,10 +42,6 @@
MachO::MachO(const Driver D, const llvm::Triple Triple,
const ArgList Args)
: ToolChain(D, Triple, Args) {
- getProgramPaths().push_back(getDriver().getInstalledDir());
- if (getDriver().getInstalledDir() != getDriver().Dir)
-getProgramPaths().push_back(getDriver().Dir);
-
// We expect 'as', 'ld', etc. to be adjacent to our install dir.
getProgramPaths().push_back(getDriver().getInstalledDir());
if (getDriver().getInstalledDir() != getDriver().Dir)
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Add nominal support for 'shave' target.
I agree with Joerg about submitting the separate cleanup change separately
(feel free to just pull it out and commit it).
More detailed comments below.
Comment at: lib/Driver/ToolChains.h:875-877
@@ -874,1 +874,5 @@
+/// MoviClang - A tool chain using the compiler that the SDK installs
+/// into MV_TOOLS_DIR (copied to llvm's installdir) to perform all subcommands.
+class LLVM_LIBRARY_VISIBILITY MoviClang : public Generic_GCC {
+public:
I'm not a huge fan of the Movi prefix. Maybe just Movidius or SHAVE? If
this is only used for the shave-* triples, I'd probably go with SHAVE.
I would also suffix it with ToolChain or something. It's not about Clang, its
about the toolchain. (The use of 'GCC' here is because GCC actually ships a
complete toolchain)
Comment at: lib/Driver/ToolChains.h:895-896
@@ +894,4 @@
+private:
+ mutable std::unique_ptrTool moviCompile;
+ mutable std::unique_ptrTool moviAsm;
+};
I suspect these should be named MoviCompile and MoviAsm. I would also probably
call these Compiler and Assembler because I'm a bit of a pedant. ;]
Comment at: lib/Driver/Tools.h:734-735
@@ -733,1 +733,4 @@
+/// movitool -- Directly call moviCompile and moviAsm
+namespace movitool {
+class LLVM_LIBRARY_VISIBILITY Compile : public Tool {
Similar to the above, I'd probably call this SHAVE given that some of the ones
above are XCore.
http://reviews.llvm.org/D10440
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Improve formatting of lambda functions.
I think we should do this, and I'll try to give a plausibly principled
rationale.
We have two uses of {}s that I care about: braced init lists and blocks. For
braced init lists, we routinely omit padding spaces around the {}s:
foo({x, y z});
return {{1, 2, 3}};
On the other hand, with every block example I can come up with, we have spaces
around the {}s:
if (...) {
}
int f() {
}
auto f() {
}
auto f() - int {
}
[] {}, []() {}, []() mutable {}, []() - int {}
It makes much more sense to me to have a space even in the presence of a
pointer (or reference).
http://reviews.llvm.org/D10410
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Improve formatting of lambda functions.
In all of these cases, the cause of the spacing is not the {. We put spaces
on both sides of a multiply, or after a comma.
The debug thing is (IMO) a bad example in every way. It's a macro, the
contents are a block, and yuck.
On Fri, Jun 12, 2015 at 1:00 AM Daniel Jasper djas...@google.com wrote:
Those examples are very selective, though:
someFunction(a, {1, 2, 3});
Debug({ llvm::errs() abc\n; });
SomeIntType b = a * {7};
http://reviews.llvm.org/D10410
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Improve formatting of lambda functions.
int *f(); // no braces, irrelevant
int *f() { // space before the brace
auto f() { // space before the brace
auto f() - int { // space before the brace
auto f() - int * { // space before the brace
On Fri, Jun 12, 2015 at 1:16 AM Daniel Jasper djas...@google.com wrote:
Ok. To be clear, I don't care strongly at all. If the two of you are
convinced this is better, lets just put it in.
How does this relate to int *f();? Do we simply not care about that
consistency at this point as this is about spacing out braces?
http://reviews.llvm.org/D10410
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] CMake targets for building man pages
I think the logic here should be bundled up in a macro in the LLVM cmake tree so that we can use it both for Clang's man page generation but also generating other man pages. Does that make sense? http://reviews.llvm.org/D10304 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Make ToolChain::SelectTool virtual.
Folks on cfe-dev seem happy with this direction, so go ahead and submit with a tweaked comment as below... Comment at: include/clang/Driver/ToolChain.h:166-171 @@ -165,3 +165,8 @@ /// Choose a tool to use to handle the action \p JA. - Tool *SelectTool(const JobAction JA) const; + /// This is virtualized because there are scenarios where you want the + /// default driver in the default driver-mode not to pick Clang as + /// the right tool for a C program, at the discretion of the ToolChain. + /// GetTool is overridable, but it's also necessary to avoid inquiring with + /// Driver::ShouldUseClangCompiler() which says yes to C, usually. + virtual Tool *SelectTool(const JobAction JA) const; Sorry, my inline comment didn't take the first time. I feel like this comment can be more direct, and avoid the weird virtualized wording. How about something like this: /// Choose a tool to handle the action \p JA. /// /// This can be overridden when a particular ToolChain needs to use /// a C compiler other than Clang. I think going into Driver::ShouldUseClangCompiler is more appropriate for the change description than a comment, it isn't really useful documentation to the reader of the interface. http://reviews.llvm.org/D10246 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r238823 - Make vim also output a helpful message in some error cases.
It's possible. I don't think my script and my clang format binary are
version locked. Should they be? Is that important?
On Thu, Jun 4, 2015, 21:45 Manuel Klimek kli...@google.com wrote:
Were you on an outdated version of clang-format?
On Thu, Jun 4, 2015, 11:27 PM Chandler Carruth chandl...@google.com
wrote:
It's because the dict doesn't always have the key.
Fixed in r239098.
On Thu, Jun 4, 2015 at 2:21 PM Chandler Carruth chandl...@google.com
wrote:
After this, all my code produces this error... The message isn't very
helpful, how should I debug this?
On Tue, Jun 2, 2015 at 5:08 AM Manuel Klimek kli...@google.com wrote:
Author: klimek
Date: Tue Jun 2 07:01:50 2015
New Revision: 238823
URL: http://llvm.org/viewvc/llvm-project?rev=238823view=rev
Log:
Make vim also output a helpful message in some error cases.
When clang-format encounters a syntax error, it will not format that
line; we're now using the same mechanism we're already using in emacs to
show a helpful error message to the user.
Modified:
cfe/trunk/tools/clang-format/clang-format.py
Modified: cfe/trunk/tools/clang-format/clang-format.py
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-format/clang-format.py?rev=238823r1=238822r2=238823view=diff
==
--- cfe/trunk/tools/clang-format/clang-format.py (original)
+++ cfe/trunk/tools/clang-format/clang-format.py Tue Jun 2 07:01:50
2015
@@ -85,6 +85,8 @@ def main():
for op in reversed(sequence.get_opcodes()):
if op[0] is not 'equal':
vim.current.buffer[op[1]:op[2]] = lines[op[3]:op[4]]
+if output['IncompleteFormat']:
+ print 'clang-format: incomplete (syntax errors)'
vim.command('goto %d' % (output['Cursor'] + 1))
main()
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Make ToolChain::SelectTool virtual.
Suggested some improvements to the comments below. Generally, this makes sense, but I think you should also mention your end-goal in the commit log. I happen to know because we've chatted, and so this change makes sense to me, but others won't have that context. For those reading the review log, the context is that there exist some embedded platforms which use specialized C derivatives and C compilers where Clang doesn't support their derivative yet. We'd like to add support for these platforms to Clang, but at least until we do, its useful if the toolchain for such an embedded target can detect special flags and dispatch to whatever embedded compiler is commonly used. This makes the Clang driver more of a drop-in replacement and starts us down the path of support. Actually, Doug, it might be worth sending an email to cfe-dev with the overall plan you have here to support C-dialects on embedded platforms by dispatching to the vendor-provided SDK until Clang grows support for that dialect. There you can give an overview and hopefully that'll provide context for folks seeing these commits. http://reviews.llvm.org/D10246 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r238823 - Make vim also output a helpful message in some error cases.
After this, all my code produces this error... The message isn't very
helpful, how should I debug this?
On Tue, Jun 2, 2015 at 5:08 AM Manuel Klimek kli...@google.com wrote:
Author: klimek
Date: Tue Jun 2 07:01:50 2015
New Revision: 238823
URL: http://llvm.org/viewvc/llvm-project?rev=238823view=rev
Log:
Make vim also output a helpful message in some error cases.
When clang-format encounters a syntax error, it will not format that
line; we're now using the same mechanism we're already using in emacs to
show a helpful error message to the user.
Modified:
cfe/trunk/tools/clang-format/clang-format.py
Modified: cfe/trunk/tools/clang-format/clang-format.py
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-format/clang-format.py?rev=238823r1=238822r2=238823view=diff
==
--- cfe/trunk/tools/clang-format/clang-format.py (original)
+++ cfe/trunk/tools/clang-format/clang-format.py Tue Jun 2 07:01:50 2015
@@ -85,6 +85,8 @@ def main():
for op in reversed(sequence.get_opcodes()):
if op[0] is not 'equal':
vim.current.buffer[op[1]:op[2]] = lines[op[3]:op[4]]
+if output['IncompleteFormat']:
+ print 'clang-format: incomplete (syntax errors)'
vim.command('goto %d' % (output['Cursor'] + 1))
main()
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r238823 - Make vim also output a helpful message in some error cases.
It's because the dict doesn't always have the key.
Fixed in r239098.
On Thu, Jun 4, 2015 at 2:21 PM Chandler Carruth chandl...@google.com
wrote:
After this, all my code produces this error... The message isn't very
helpful, how should I debug this?
On Tue, Jun 2, 2015 at 5:08 AM Manuel Klimek kli...@google.com wrote:
Author: klimek
Date: Tue Jun 2 07:01:50 2015
New Revision: 238823
URL: http://llvm.org/viewvc/llvm-project?rev=238823view=rev
Log:
Make vim also output a helpful message in some error cases.
When clang-format encounters a syntax error, it will not format that
line; we're now using the same mechanism we're already using in emacs to
show a helpful error message to the user.
Modified:
cfe/trunk/tools/clang-format/clang-format.py
Modified: cfe/trunk/tools/clang-format/clang-format.py
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-format/clang-format.py?rev=238823r1=238822r2=238823view=diff
==
--- cfe/trunk/tools/clang-format/clang-format.py (original)
+++ cfe/trunk/tools/clang-format/clang-format.py Tue Jun 2 07:01:50 2015
@@ -85,6 +85,8 @@ def main():
for op in reversed(sequence.get_opcodes()):
if op[0] is not 'equal':
vim.current.buffer[op[1]:op[2]] = lines[op[3]:op[4]]
+if output['IncompleteFormat']:
+ print 'clang-format: incomplete (syntax errors)'
vim.command('goto %d' % (output['Cursor'] + 1))
main()
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r239098 - Fix terrible python goof in clang-format.py which broke my vim
Author: chandlerc
Date: Thu Jun 4 16:23:07 2015
New Revision: 239098
URL: http://llvm.org/viewvc/llvm-project?rev=239098view=rev
Log:
Fix terrible python goof in clang-format.py which broke my vim
integration.
Nothing is more important in life than clang-format integration with
vim. ;]
Modified:
cfe/trunk/tools/clang-format/clang-format.py
Modified: cfe/trunk/tools/clang-format/clang-format.py
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-format/clang-format.py?rev=239098r1=239097r2=239098view=diff
==
--- cfe/trunk/tools/clang-format/clang-format.py (original)
+++ cfe/trunk/tools/clang-format/clang-format.py Thu Jun 4 16:23:07 2015
@@ -85,7 +85,7 @@ def main():
for op in reversed(sequence.get_opcodes()):
if op[0] is not 'equal':
vim.current.buffer[op[1]:op[2]] = lines[op[3]:op[4]]
-if output['IncompleteFormat']:
+if output.get('IncompleteFormat'):
print 'clang-format: incomplete (syntax errors)'
vim.command('goto %d' % (output['Cursor'] + 1))
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r238389 - [omp] Re-work Clang's handling of -fopenmp and undo r237769.
On Fri, May 29, 2015 at 11:40 AM İsmail Dönmez ism...@donmez.ws wrote: On Fri, May 29, 2015 at 9:32 PM, Richard Smith rich...@metafoo.co.uk wrote: On Fri, May 29, 2015 at 5:56 AM, İsmail Dönmez ism...@donmez.ws wrote: On Thu, May 28, 2015 at 11:48 PM, Chandler Carruth chandl...@gmail.com wrote: On Thu, May 28, 2015 at 5:09 AM İsmail Dönmez ism...@donmez.ws wrote: Hi, On Thu, May 28, 2015 at 4:52 AM, Chandler Carruth chandl...@gmail.com wrote: Author: chandlerc Date: Wed May 27 20:52:38 2015 New Revision: 238389 URL: http://llvm.org/viewvc/llvm-project?rev=238389view=rev Log: [omp] Re-work Clang's handling of -fopenmp and undo r237769. This seems to break openmp tests with -DCLANG_DEFAULT_OPENMP_RUNTIME=libiomp5 see: Yea, I wanted to have test coverage of the default, but it doesn't work well when the default is overridden. I looked into how to actually have test coverage of the default and track which default was selected, but its really hard and complex. Yuck. I'll look for a way to get at least *some* coverage of the default without making it break folks that are overriding it. Sorry for the trouble. Tests pass now but there is still one grave problem. With CLANG_DEFAULT_OPENMP_RUNTIME=libiomp5 if you do: λ clang -fopenmp=libgomp omp_hello.c λ ./a.out Hello World from thread = 0 Number of threads = 1 No OpenMP, lets go back to defaults: λ clang -fopenmp=libiomp5 omp_hello.c λ ./a.out Hello World from thread = 0 Hello World from thread = 4 Hello World from thread = 3 Hello World from thread = 2 Hello World from thread = 6 Hello World from thread = 1 Hello World from thread = 5 Hello World from thread = 7 Number of threads = 8 OK, and what's the one grave problem? tl;dr clang -fopenmp=libgomp compiles and links to libgomp but doesn't enable OpenMP. I think that is actually the best behavior we can provide. Clang doesn't support emitting code compatible with libgomp. There is currently no support for actually enabling OpenMP in Clang and using libgomp. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r238389 - [omp] Re-work Clang's handling of -fopenmp and undo r237769.
On Thu, May 28, 2015 at 5:09 AM İsmail Dönmez ism...@donmez.ws wrote: Hi, On Thu, May 28, 2015 at 4:52 AM, Chandler Carruth chandl...@gmail.com wrote: Author: chandlerc Date: Wed May 27 20:52:38 2015 New Revision: 238389 URL: http://llvm.org/viewvc/llvm-project?rev=238389view=rev Log: [omp] Re-work Clang's handling of -fopenmp and undo r237769. This seems to break openmp tests with -DCLANG_DEFAULT_OPENMP_RUNTIME=libiomp5 see: Yea, I wanted to have test coverage of the default, but it doesn't work well when the default is overridden. I looked into how to actually have test coverage of the default and track which default was selected, but its really hard and complex. Yuck. I'll look for a way to get at least *some* coverage of the default without making it break folks that are overriding it. Sorry for the trouble. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r238498 - [omp] Fix a typo in a comment and a line I forgot to clang-format that
Author: chandlerc
Date: Thu May 28 16:10:31 2015
New Revision: 238498
URL: http://llvm.org/viewvc/llvm-project?rev=238498view=rev
Log:
[omp] Fix a typo in a comment and a line I forgot to clang-format that
Justin pointed out in post-commit review.
Modified:
cfe/trunk/lib/Driver/Tools.cpp
Modified: cfe/trunk/lib/Driver/Tools.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=238498r1=238497r2=238498view=diff
==
--- cfe/trunk/lib/Driver/Tools.cpp (original)
+++ cfe/trunk/lib/Driver/Tools.cpp Thu May 28 16:10:31 2015
@@ -2293,7 +2293,7 @@ enum OpenMPRuntimeKind {
/// runtime library itself.
OMPRT_GOMP,
- /// The legacy name for the LLVM OpenMP runtim from when it was the Intel
+ /// The legacy name for the LLVM OpenMP runtime from when it was the Intel
/// OpenMP runtime. We support this mode for users with existing dependencies
/// on this runtime library name.
OMPRT_IOMP5
@@ -2301,7 +2301,8 @@ enum OpenMPRuntimeKind {
}
/// Compute the desired OpenMP runtime from the flag provided.
-static OpenMPRuntimeKind getOpenMPRuntime(const ToolChain TC, const ArgList
Args) {
+static OpenMPRuntimeKind getOpenMPRuntime(const ToolChain TC,
+ const ArgList Args) {
StringRef RuntimeName(CLANG_DEFAULT_OPENMP_RUNTIME);
const Arg *A = Args.getLastArg(options::OPT_fopenmp_EQ);
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r238500 - [omp] Loosen the driver test enough so that overriding the defaults
Author: chandlerc
Date: Thu May 28 16:20:14 2015
New Revision: 238500
URL: http://llvm.org/viewvc/llvm-project?rev=238500view=rev
Log:
[omp] Loosen the driver test enough so that overriding the defaults
works well for folks.
This isn't terribly clean (sadly) but after chatting with both Eric and
Richard, nothing cleaner really emerged. The clean way of doing this is
a *lot* of work for extremely little benefit here.
Modified:
cfe/trunk/test/Driver/fopenmp.c
Modified: cfe/trunk/test/Driver/fopenmp.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/fopenmp.c?rev=238500r1=238499r2=238500view=diff
==
--- cfe/trunk/test/Driver/fopenmp.c (original)
+++ cfe/trunk/test/Driver/fopenmp.c Thu May 28 16:20:14 2015
@@ -1,4 +1,3 @@
-// RUN: %clang -target x86_64-linux-gnu -fopenmp -c %s -### 21 | FileCheck
%s --check-prefix=CHECK-CC1-NO-OPENMP
// RUN: %clang -target x86_64-linux-gnu -fopenmp=libomp -c %s -### 21 |
FileCheck %s --check-prefix=CHECK-CC1-OPENMP
// RUN: %clang -target x86_64-linux-gnu -fopenmp=libgomp -c %s -### 21 |
FileCheck %s --check-prefix=CHECK-CC1-NO-OPENMP
// RUN: %clang -target x86_64-linux-gnu -fopenmp=libiomp5 -c %s -### 21 |
FileCheck %s --check-prefix=CHECK-CC1-OPENMP
@@ -9,7 +8,6 @@
// CHECK-CC1-NO-OPENMP: -cc1
// CHECK-CC1-NO-OPENMP-NOT: -fopenmp
//
-// RUN: %clang -target x86_64-linux-gnu -fopenmp %s -o %t -### 21 |
FileCheck %s --check-prefix=CHECK-LD-GOMP
// RUN: %clang -target x86_64-linux-gnu -fopenmp=libomp %s -o %t -### 21 |
FileCheck %s --check-prefix=CHECK-LD-OMP
// RUN: %clang -target x86_64-linux-gnu -fopenmp=libgomp %s -o %t -### 21 |
FileCheck %s --check-prefix=CHECK-LD-GOMP
// RUN: %clang -target x86_64-linux-gnu -fopenmp=libiomp5 %s -o %t -### 21 |
FileCheck %s --check-prefix=CHECK-LD-IOMP5
@@ -22,3 +20,14 @@
//
// CHECK-LD-IOMP5: {{.*}}ld{{(.exe)?}}
// CHECK-LD-IOMP5: -liomp5
+//
+// We'd like to check that the default is sane, but until we have the ability
+// to *always* semantically analyze OpenMP without always generating runtime
+// calls (in the event of an unsupported runtime), we don't have a good way to
+// test the CC1 invocation. Instead, just ensure we do eventually link *some*
+// OpenMP runtime.
+//
+// RUN: %clang -target x86_64-linux-gnu -fopenmp %s -o %t -### 21 |
FileCheck %s --check-prefix=CHECK-LD-ANY
+//
+// CHECK-LD-ANY: {{.*}}ld{{(.exe)?}}
+// CHECK-LD-ANY: -l{{(omp|gomp|iomp5)}}
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r238389 - [omp] Re-work Clang's handling of -fopenmp and undo r237769.
On Thu, May 28, 2015 at 2:46 PM Andrey Bokhanko andreybokha...@gmail.com wrote: Chandler, Issue #1 -- not an issue at all. Personally, I *welcome* your fixes, and it's logical they are coming from you -- as apart of users you mentioned, there are zero documented users who *rely* on clang linking libgomp and ignoring all OMP pragmas. As for lack of code review, I trust your judgment and experience here (and still (!) ready to learn from you... -- remember?) and more than trust your driver fu. Issue #2 -- yes, I'm concerned. This seriously affects the shortest path user interface to OMP implementation and runtime library. I believe it warrants at least a discussion and some kind of consent in the community. Obviously, involving code owners of said OMP implementations would be reasonable. I think that trunk should continue working for existing users during that discussion. I also think the discussion is on-going as the OpenMP runtime issues are being addressed. I don't any particular problem with the progress there, I just think that the default flip should not happen until either: a) The work completes and it is clearly working well for users to get both Clang and the new runtime and use them together, or b) There is some widespread community desire to *force* the default to change and break existing users until the pieces are fit back together with the new runtime. I don't like (b) and would argue against it, but I also haven't seen anyone who really does like (b). I don't believe this meaningfully impacts the cost of users getting at the OMP implementation -- before Alexey's patch, they had to pass -fopenmp=libiomp5, now they still do. If anything, it is still easier as they can just have CMake remap the default. The default flip actually regressed a very large number of my users. Regressing users is *not* OK. Given the lack of response from the author, I think I was entirely justified returning the default to its previous state. That's surprising. We discussed this with Alexey and the very next day he responded with a fix that (as we hoped) resolves your concerns and gives an easy way to configure clang the way you and your users like. You replied to this fix with: Yea, i think Daniel's patch was a closer start. Anyways, thanks for taking a stab at this. (from http://reviews.llvm.org/D9875) I interpreted it as I'm not 100% happy but that's OK. Is my interpretation wrong? This only addressed one of the several complaints I raised in the review thread. And the review thread itself was never updated. I *really* don't want us to become two boys shouting it is libgomp! flip and no, it is libiomp! flip to each other and flipping the switch ad infinitum. But I frankly fail to understand your logic... I don't think that's going to happen -- I think the patch to flip the default just went in too soon. FTR, given the progress on the CMake and lit side of things, I wouldn't expect this to even be risky for 3.7... The logic I'm using is that trunk should stay *green*, and should work out-of-the box for users. Until the OpenMP runtimes work out of the box, we shouldn't flip the default. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r238389 - [omp] Re-work Clang's handling of -fopenmp and undo r237769.
Thanks for the review!
I'll make these changes shortly, just responding below:
On Thu, May 28, 2015 at 12:39 AM Justin Bogner m...@justinbogner.com
wrote:
Chandler Carruth chandl...@gmail.com writes:
+ /// The legacy name for the LLVM OpenMP runtim from when it was the
Intel
^~
Typo.
Good catch!
+ /// OpenMP runtime. We support this mode for users with existing
dependencies
+ /// on this runtime library name.
+ OMPRT_IOMP5
+};
+}
+
+/// Compute the desired OpenMP runtime from the flag provided.
+static OpenMPRuntimeKind getOpenMPRuntime(const ToolChain TC, const
ArgList Args) {
Long line?
Doh, will fix.
+ StringRef RuntimeName(CLANG_DEFAULT_OPENMP_RUNTIME);
+
+ const Arg *A = Args.getLastArg(options::OPT_fopenmp_EQ);
+ if (A)
+RuntimeName = A-getValue();
+
+ auto RT = llvm::StringSwitchOpenMPRuntimeKind(RuntimeName)
+ .Case(libomp, OMPRT_OMP)
+ .Case(libgomp, OMPRT_GOMP)
+ .Case(libiomp5, OMPRT_IOMP5)
+ .Default(OMPRT_Unknown);
+
+ if (RT == OMPRT_Unknown) {
+if (A)
+ TC.getDriver().Diag(diag::err_drv_unsupported_option_argument)
+ A-getOption().getName() A-getValue();
+else
+ // FIXME: We could use a nicer diagnostic here.
+ TC.getDriver().Diag(diag::err_drv_unsupported_opt) -fopenmp;
Is this even reachable in a meaningful way? CLANG_DEFAULT_OPENMP_RUNTIME
has to be something we don't understand. If we can somehow get here, the
error is really You're using a version of LLVM that was configured
wrong, get a better one.
Totally open to suggestions about how to approach this. I wasn't sure of
the best one myself, and left a FIXME to try and find a better one. At the
least, as you say, this isn't really reachable in any sensible setup. Ideas
about how to actually check this?
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r238389 - [omp] Re-work Clang's handling of -fopenmp and undo r237769.
On Thu, May 28, 2015 at 5:48 AM Andrey Bokhanko andreybokha...@gmail.com wrote: Chandler, +set(CLANG_DEFAULT_OPENMP_RUNTIME libgomp CACHE STRING + Default OpenMP runtime used by -fopenmp.) This effectively invalidates instructions recently published on LLVM blog (http://blog.llvm.org/2015/05/openmp-support_22.html) and picked up in many places. I don't think there is anything we can do about this. =/ That's the risk of publishing widely about stuff that has just barely landed in the tree. To re-enable LLVM's OpenMP runtime (which I think should wait until the normal getting started instructions are a reasonable way for falks to check out, build, and install Clang with the runtime) Why this readme: http://openmp.llvm.org/README.txt (easily accessible from openmp.llvm.org) is not sufficient? Why would someone trying to get LLVM or Clang read the OpenMP readme? Clang *directly* supports the '-fopenmp' flag, so I wouldn't expect them to go read other files. Look at the instructions around compiler-rt -- we're very clear that parts of Clang actually rely on these runtime libraries. I'll even help update all of these documents once the cleanups and fixes to the CMake and lit infrastructure land, and it's reasonably unintrusive to grab the OpenMP runtime along with Clang and LLVM and build all of it. Also, have you reviewed your patch with OpenMP in Clang code owner (Alexey Bataev)? No, and that is never a requirement really. Code owners don't have *exclusive* rights to approve patches. That's not the point of a code owner in LLVM. They are a person of last resort if a contributor can't find someone else to review the patch. Or at all?... I have worked *extensively* on the Clang driver and am very comfortable committing these kinds of improvements and getting post-commit review. And you'll see some nice post commit review on this thread. I'll point out that my patch added significant missing testing, fixed numerous problems, and had much more comprehensive documentation than the original patch. There are ultimately two separate issues here: 1) The *mechanisms* for controlling openmp support were not correctly implemented and needed to be restructured, fixed, and tested. This patch does all of that, and 99.9% of it is concerned with these aspects. 2) The default was flipped back to not relying on the iomp5 runtime. I don't think #1 is controversial at all, but if you do, we can discuss that more. I think #2 may be a bit controversial, but I actually tried to reach out to the person who flipped the default the first time -- Alexey -- in post-commit review before doing anything. I did this *over a week ago*. There was no response on that thread, and nothing was done to address the concerns I raised. The default flip actually regressed a very large number of my users. Regressing users is *not* OK. Given the lack of response from the author, I think I was entirely justified returning the default to its previous state. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] [clang] Update a test-case affected by IntToPtr/PtrToInt being folded into Loads
Looks good (assuming with the LLVM change). REPOSITORY rL LLVM http://reviews.llvm.org/D9153 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r237769 - [OPENMP] -fopenmp enables OpenMP support (fix for http://llvm.org/PR23492)
On Wed, May 27, 2015 at 3:51 PM Chandler Carruth chandl...@google.com wrote: On Wed, May 20, 2015 at 12:02 AM Chandler Carruth chandl...@google.com wrote: I don't think this should have been committed yet. It's been a week without response. 1) The CMake build for the OpenMP stuff is still really broken and not well integrated with the rest of the LLVM project 2) Because of #1, I suspect very few users of Clang and LLVM have the libraries installed. 3) If they do, its still called 'libiomp' and not the final name that we planned for. None of these issues have been addressed. Now, I have users that have libgomp installed and have been successfully using '-fopenmp' to link against it for over a year in order to satisfy the runtime library calls against its API. With this patch, all of them are broken because we haven't yet been able to build and deploy the LLVM OpenMP library to them, largely because of the above problems. I'd much rather this not go in yet, or at least expose some CMake variable to revert to the old behavior so we can continue supporting users that had a working setup before this patch and now are broken. This at least got partially fixed, but that fix didn't work because this change has another, *much* more significant change that was never mentioned in the review or the change description. Before this change, using '-fopenmp' as a flag to GCC enabled OpenMP and linked against libgomp. Using '-fopenmp' as a flag to Clang linked against libgomp, but *did not enable any OpenMP language features*. Let me restate that because it took me a couple of tries to really get this: everyone using '-fopenmp' as a flag to Clang has had working compiles but *zero* actual usage of Clang's OpenMP support until this commit. As a consequence, any bugs or problems in Clang's OpenMP language support would only get diagnosed of users of Clang were explicitly passing '-fopenmp=libiomp5'. Also as a consequence, prior to this patch '-fopenmp' as a flag to Clang enabled a tested, working, but *very* slow OpenMP implementation combined with libgomp. Clang nicely ignored every pragma, and generated serial code. After this change, users got a broken build no matter what they did. There is no easy way to recover the old behavior because now the language level facilities are enabled, but there is no hook to disable *code generation* separately from the parsing and semantic analysis. So, given the silence here and the amount of work left to allow random Jane developer to grab LLVM and Clang and necessary runtimes to build, install, and use a toolchain supporting OpenMP, I'm going to take the default back. I'm also going to add infrastructure to make it easy to use '-fopenmp=libgomp' or a CMake variable to retain the previous behavior. I would love to have an enhancement of this that would actually *only* remove the IR generation for the openmp constructs, but I don't have time to implement that. All of this was committed in r238389. Sorry for the trouble, but we've had users broken by this for a week now. -Chandler ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r238389 - [omp] Re-work Clang's handling of -fopenmp and undo r237769.
Author: chandlerc
Date: Wed May 27 20:52:38 2015
New Revision: 238389
URL: http://llvm.org/viewvc/llvm-project?rev=238389view=rev
Log:
[omp] Re-work Clang's handling of -fopenmp and undo r237769.
This isn't an actual revert of r237769, it just restores the behavior of
the Clang driver prior to it while completely re-implementing how that
behavior works.
This also re-does the work of making the default OpenMP runtime
selectable at CMake (or configure) time to work in the way all of our
other such hooks do (config.h, configure and cmake hooks, etc.).
I've re-implemented how we manage the '-fopenmp' flagset in an important
way. Now, the default hook just makes '-fopenmp' equivalent to
'-fopenmp=default' rather than a separate special beast. Also, there
is an '-fno-openmp' flag which does the obvious thing. Also, the code is
shared between all the places to select a known OpenMP runtime and act
on it.
Finally, and most significantly, I've taught the driver to inspect the
selected runtime when choosing whether to propagate the '-fopenmp' flag
to the frontend in the CC1 commandline. Without this, it isn't possible
to use Clang with libgomp, even if you were happy with the serial,
boring way in which it worked previously (ignoring all #pragmas but
linking in the library to satisfy direct calls into the runtime).
While I'm here, I've gone ahead and sketched out a path for the future
name of LLVM's OpenMP runtime (libomp) and the legacy support for its
current name (libiomp5) in what seems a more reasonable way.
To re-enable LLVM's OpenMP runtime (which I think should wait until the
normal getting started instructions are a reasonable way for falks to
check out, build, and install Clang with the runtime) all that needs to
change is the default string in the CMakeLists.txt and configure.ac
file. No code changes necessary.
I also added a test for the driver's behavior around OpenMP since it was
*completely missing* previously. Makes it unsurprising that we got it
wrong.
Added:
cfe/trunk/test/Driver/fopenmp.c
Modified:
cfe/trunk/CMakeLists.txt
cfe/trunk/include/clang/Config/config.h.cmake
cfe/trunk/include/clang/Config/config.h.in
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/lib/Driver/Tools.cpp
Modified: cfe/trunk/CMakeLists.txt
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/CMakeLists.txt?rev=238389r1=238388r2=238389view=diff
==
--- cfe/trunk/CMakeLists.txt (original)
+++ cfe/trunk/CMakeLists.txt Wed May 27 20:52:38 2015
@@ -182,6 +182,9 @@ set(GCC_INSTALL_PREFIX CACHE PATH Di
set(DEFAULT_SYSROOT CACHE PATH
Default path to all compiler invocations for --sysroot=path. )
+set(CLANG_DEFAULT_OPENMP_RUNTIME libgomp CACHE STRING
+ Default OpenMP runtime used by -fopenmp.)
+
set(CLANG_VENDOR CACHE STRING
Vendor-specific text for showing with version information.)
@@ -441,11 +444,6 @@ if(CLANG_ENABLE_STATIC_ANALYZER)
add_definitions(-DCLANG_ENABLE_STATIC_ANALYZER)
endif()
-set(OPENMP_DEFAULT_LIB CACHE STRING OpenMP library used by default for
-fopenmp.)
-if(OPENMP_DEFAULT_LIB)
- add_definitions(-DOPENMP_DEFAULT_LIB=${OPENMP_DEFAULT_LIB})
-endif()
-
# Clang version information
set(CLANG_EXECUTABLE_VERSION
${CLANG_VERSION_MAJOR}.${CLANG_VERSION_MINOR} CACHE STRING
Modified: cfe/trunk/include/clang/Config/config.h.cmake
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Config/config.h.cmake?rev=238389r1=238388r2=238389view=diff
==
--- cfe/trunk/include/clang/Config/config.h.cmake (original)
+++ cfe/trunk/include/clang/Config/config.h.cmake Wed May 27 20:52:38 2015
@@ -8,6 +8,9 @@
/* Bug report URL. */
#define BUG_REPORT_URL ${BUG_REPORT_URL}
+/* Default OpenMP runtime used by -fopenmp. */
+#define CLANG_DEFAULT_OPENMP_RUNTIME ${CLANG_DEFAULT_OPENMP_RUNTIME}
+
/* Multilib suffix for libdir. */
#define CLANG_LIBDIR_SUFFIX ${CLANG_LIBDIR_SUFFIX}
Modified: cfe/trunk/include/clang/Config/config.h.in
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Config/config.h.in?rev=238389r1=238388r2=238389view=diff
==
--- cfe/trunk/include/clang/Config/config.h.in (original)
+++ cfe/trunk/include/clang/Config/config.h.in Wed May 27 20:52:38 2015
@@ -8,6 +8,9 @@
/* Bug report URL. */
#undef BUG_REPORT_URL
+/* Default OpenMP runtime used by -fopenmp. */
+#undef CLANG_DEFAULT_OPENMP_RUNTIME
+
/* Multilib suffix for libdir. */
#undef CLANG_LIBDIR_SUFFIX
Modified: cfe/trunk/include/clang/Driver/Options.td
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=238389r1=238388r2=238389view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++
Re: r237769 - [OPENMP] -fopenmp enables OpenMP support (fix for http://llvm.org/PR23492)
On Wed, May 20, 2015 at 12:02 AM Chandler Carruth chandl...@google.com wrote: I don't think this should have been committed yet. It's been a week without response. 1) The CMake build for the OpenMP stuff is still really broken and not well integrated with the rest of the LLVM project 2) Because of #1, I suspect very few users of Clang and LLVM have the libraries installed. 3) If they do, its still called 'libiomp' and not the final name that we planned for. None of these issues have been addressed. Now, I have users that have libgomp installed and have been successfully using '-fopenmp' to link against it for over a year in order to satisfy the runtime library calls against its API. With this patch, all of them are broken because we haven't yet been able to build and deploy the LLVM OpenMP library to them, largely because of the above problems. I'd much rather this not go in yet, or at least expose some CMake variable to revert to the old behavior so we can continue supporting users that had a working setup before this patch and now are broken. This at least got partially fixed, but that fix didn't work because this change has another, *much* more significant change that was never mentioned in the review or the change description. Before this change, using '-fopenmp' as a flag to GCC enabled OpenMP and linked against libgomp. Using '-fopenmp' as a flag to Clang linked against libgomp, but *did not enable any OpenMP language features*. Let me restate that because it took me a couple of tries to really get this: everyone using '-fopenmp' as a flag to Clang has had working compiles but *zero* actual usage of Clang's OpenMP support until this commit. As a consequence, any bugs or problems in Clang's OpenMP language support would only get diagnosed of users of Clang were explicitly passing '-fopenmp=libiomp5'. Also as a consequence, prior to this patch '-fopenmp' as a flag to Clang enabled a tested, working, but *very* slow OpenMP implementation combined with libgomp. Clang nicely ignored every pragma, and generated serial code. After this change, users got a broken build no matter what they did. There is no easy way to recover the old behavior because now the language level facilities are enabled, but there is no hook to disable *code generation* separately from the parsing and semantic analysis. So, given the silence here and the amount of work left to allow random Jane developer to grab LLVM and Clang and necessary runtimes to build, install, and use a toolchain supporting OpenMP, I'm going to take the default back. I'm also going to add infrastructure to make it easy to use '-fopenmp=libgomp' or a CMake variable to retain the previous behavior. I would love to have an enhancement of this that would actually *only* remove the IR generation for the openmp constructs, but I don't have time to implement that. Sorry for the trouble, but we've had users broken by this for a week now. -Chandler ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [libcxxabi] r238263 - Make sure !empty() before calling String::front().
On Tue, May 26, 2015 at 7:54 PM David Majnemer david.majne...@gmail.com
wrote:
On Tue, May 26, 2015 at 7:22 PM, Chaoren Lin chaor...@google.com wrote:
Hi David,
Thanks for your concern.
The commit was intentional. I was hoping this small commit would fall
under this part:
smaller changes can be reviewed after commit
of the LLVM Developer Policy
https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_docs_DeveloperPolicy.html-23code-2Dreviewsd=AwMFaQc=8hUWFZcy2Z-Za5rBPlktOQr=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxkm=QuAaZphRKuyHP5n6oQVKXZjT9Q798WxBKrlU6YLePNIs=ohuT68Er4qKRMq9onem2NyWK1BuTEIbtX1YABHqMsyse=
I'm afraid it doesn't because of the following proviso:
Code review can be an iterative process, which continues until the patch
is ready to be committed. Specifically, once a patch is sent out for
review, it needs an explicit “looks good” before it is submitted. Do not
assume silent approval, or request active objections to the patch with a
deadline.
This section is just confusing.
Slightly better wording is here:
http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
This establishes the guideline of 'obvious'. I don't think any change I
make (other than fixing the spelling of a comment) to a project I don't
routinely contribute to would be obvious to me.
The small and obvious wording aren't clear (I'll work to update the
docs here) but I think they are supposed to cover the same set of changes,
and I don't think they would cover any changes to a project you have little
or no experience with previously.
We inlined a copy of this file in LLDB for Windows, and I ran into a
problem where any mangled names with pointers or references in their
argument list would cause a crash. Turns out that on Linux, String::front()
consistently returns '\0' when empty, even though it should be undefined
behavior.
It should be covered by existing tests if you enable asserts for libcxx
and try to demangle _Z1FPiRiOiMS0_FiiE.
I don't see an existing test which tries to demangle
'_Z1FPiRiOiMS0_FiiE'. I think you should add it
to test/test_demangle.pass.cpp
Thanks,
Chaoren
On Tue, May 26, 2015 at 4:43 PM, David Majnemer david.majne...@gmail.com
wrote:
I didn't see anyone LGTM the phabricator review and no test has been
added for this change. Perhaps you accidentally committed this?
On Tue, May 26, 2015 at 4:14 PM, Chaoren Lin chaor...@google.com
wrote:
Author: chaoren
Date: Tue May 26 18:14:26 2015
New Revision: 238263
URL: http://llvm.org/viewvc/llvm-project?rev=238263view=rev
https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D238263-26view-3Drevd=AwMFaQc=8hUWFZcy2Z-Za5rBPlktOQr=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxkm=QuAaZphRKuyHP5n6oQVKXZjT9Q798WxBKrlU6YLePNIs=dc2o1Gcmar0Ia3dwUAnef8_iONKHEibRLQHcMRxRVZUe=
Log:
Make sure !empty() before calling String::front().
Reviewers: howard.hinnant
Subscribers: cfe-commits
Differential Revision: http://reviews.llvm.org/D9954
https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9954d=AwMFaQc=8hUWFZcy2Z-Za5rBPlktOQr=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxkm=QuAaZphRKuyHP5n6oQVKXZjT9Q798WxBKrlU6YLePNIs=14VS6SUFIr2KjQuuTaWrupP-uks0B0Rrd9sohLh_25Me=
Modified:
libcxxabi/trunk/src/cxa_demangle.cpp
Modified: libcxxabi/trunk/src/cxa_demangle.cpp
URL:
http://llvm.org/viewvc/llvm-project/libcxxabi/trunk/src/cxa_demangle.cpp?rev=238263r1=238262r2=238263view=diff
https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_libcxxabi_trunk_src_cxa-5Fdemangle.cpp-3Frev-3D238263-26r1-3D238262-26r2-3D238263-26view-3Ddiffd=AwMFaQc=8hUWFZcy2Z-Za5rBPlktOQr=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxkm=QuAaZphRKuyHP5n6oQVKXZjT9Q798WxBKrlU6YLePNIs=ygHBZOgwMcNkc7vMWXBjIett4-76FeOUf8fbbVzhxDke=
==
--- libcxxabi/trunk/src/cxa_demangle.cpp (original)
+++ libcxxabi/trunk/src/cxa_demangle.cpp Tue May 26 18:14:26 2015
@@ -1671,7 +1671,7 @@ parse_pointer_to_member_type(const char*
auto func = std::move(db.names.back());
db.names.pop_back();
auto class_type = std::move(db.names.back());
-if (func.second.front() == '(')
+if (!func.second.empty() func.second.front() == '(')
{
db.names.back().first = std::move(func.first) +
( + class_type.move_full() + ::*;
db.names.back().second = ) +
std::move(func.second);
@@ -2018,7 +2018,8 @@ parse_type(const char* first, const char
db.names[k].first += (;
db.names[k].second.insert(0, ));
}
-else if (db.names[k].second.front() ==
'(')
+else if (!db.names[k].second.empty()
+
Re: [PATCH] [OPENMP] Allow to disable OpenMP support and link libgomp
Yea, i think Daniel's patch was a closer start. Anyways, thanks for taking a stab at this. http://reviews.llvm.org/D9875 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] [OPENMP] Allow to disable OpenMP support and link libgomp
Yea, i think Daniel's patch was a closer start. Anyways, thanks for taking a stab at this. On Wed, May 20, 2015 at 8:04 PM Alexey Bataev a.bat...@hotmail.com wrote: Already has been fixed by Richard Smith. http://reviews.llvm.org/D9875 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Add 'sparcel' architecture to clang.
LGTM http://reviews.llvm.org/D8784 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Add 'sparcel' architecture to clang.
Can you add some basic testing in test/Driver for the flag handling, and in test/Preprocessor for the macros that should be predefined? http://reviews.llvm.org/D8784 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] [CodeGen] Annotate signed shift lefts with NUW
Before we go here, I think we should seriously ask whether we want to undo that restriction on shift left. I can see arguments both for and against it. Does GCC already do this optimization? http://reviews.llvm.org/D8899 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] [CodeGen] Annotate signed shift lefts with NUW
This doesn't seem correct for a negative LHS where we only shift out 1s -- I think that should be valid. Essentially, we seem to have a strange circumstance here where NUW as specified is not a subset of the restrictions of NSW. http://reviews.llvm.org/D8899 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] [X86] Use sys::getHostCPUFeatures to improve -march=native
Looks good, go for it. http://reviews.llvm.org/D8694 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Raising minimum required CMake version to 2.8.12.2.
Unsure if you were actually looking for review of this Chris, but it looks fine. http://reviews.llvm.org/D7797 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] [CMake] Resurrect add_dependencies() and target_link_libraries()
This seems stalled. Update and add me back if you want me to take a look at it. http://reviews.llvm.org/D2898 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] simplify pie/pic handling for android
Resigning as this patch has gotten quite stale. If someone is still interested, update or send a new patch. http://reviews.llvm.org/D3542 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Mingw-w64 driver for clang
I'm not a good reviewer here (I don't work on Windows) and Reid seems like an excellent reviewer. http://reviews.llvm.org/D5268 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] libclang plugin patch
I'm definitely not the right reviewer for this. Richard or Doug make much more sense. http://reviews.llvm.org/D5611 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] PR19864: Revert r209764 in favor of the fix in r215766 (loop backedge jump attributed to confusing location when the last source line of a loop is inside a conditional)
Dave, not sure there is anything else you need from me here, but if so, add me back and let me know. Until then, i'm moving this off my review queue. http://reviews.llvm.org/D4956 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Introduce _LIBCPP_ABI_UNSTABLE to libc++
FYI, as I commented on the original discussion, I'd personally like to see this have some versioning scheme before it goes in rather than a binary unstable. Or some plan for getting there. Otherwise this looks good as a first step. http://reviews.llvm.org/D6712 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Add clang-fuzzer target
Er, I believe you already submitted this with Manuel's review. In the future, please have him actually mark the review on the mailing list so its clear to others that it was in fact reviewed. http://reviews.llvm.org/D7289 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233426 - [Modules] Work around PR23030 again, in a different code path, where
Author: chandlerc
Date: Fri Mar 27 16:40:58 2015
New Revision: 233426
URL: http://llvm.org/viewvc/llvm-project?rev=233426view=rev
Log:
[Modules] Work around PR23030 again, in a different code path, where
I again added the reasonable assertions and they again fired during
a modules self-host.
This hopefully will un-break the self-host build bot. No test case handy
and adding one seems to have little or no value really.
Modified:
cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
Modified: cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriterDecl.cpp?rev=233426r1=233425r2=233426view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriterDecl.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriterDecl.cpp Fri Mar 27 16:40:58 2015
@@ -994,13 +994,16 @@ void ASTDeclWriter::VisitNamespaceDecl(N
std::sort(LookupResults.begin(), LookupResults.end(), llvm::less_first());
for (auto NameAndResult : LookupResults) {
DeclarationName Name = NameAndResult.first;
- (void)Name;
- assert(Name.getNameKind() != DeclarationName::CXXConstructorName
- Cannot have a constructor name in a namespace!);
- assert(Name.getNameKind() != DeclarationName::CXXConversionFunctionName
- Cannot have a conversion function name in a namespace!);
-
DeclContext::lookup_result Result = NameAndResult.second;
+ if (Name.getNameKind() == DeclarationName::CXXConstructorName ||
+ Name.getNameKind() == DeclarationName::CXXConversionFunctionName) {
+// We have to work around a name lookup bug here where negative lookup
+// results for these names get cached in namespace lookup tables.
+assert(Result.empty() Cannot have a constructor or conversion
+ function name in a namespace!);
+continue;
+ }
+
for (NamedDecl *ND : Result)
Writer.GetDeclRef(ND);
}
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233462 - [Modules] Don't compute a modules cache path if we're not using modules!
Author: chandlerc Date: Fri Mar 27 20:10:44 2015 New Revision: 233462 URL: http://llvm.org/viewvc/llvm-project?rev=233462view=rev Log: [Modules] Don't compute a modules cache path if we're not using modules! Notably, this prevents us from doing *tons* of work to compute the modules hash, including trying to read a darwin specific plist file off of the system. There is a lot that needs cleaning up below this layer too. Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=233462r1=233461r2=233462view=diff == --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original) +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Fri Mar 27 20:10:44 2015 @@ -329,7 +329,8 @@ void CompilerInstance::createPreprocesso PP-setPreprocessedOutput(getPreprocessorOutputOpts().ShowCPP); - PP-getHeaderSearchInfo().setModuleCachePath(getSpecificModuleCachePath()); + if (PP-getLangOpts().Modules) +PP-getHeaderSearchInfo().setModuleCachePath(getSpecificModuleCachePath()); // Handle generating dependencies, if requested. const DependencyOutputOptions DepOpts = getDependencyOutputOpts(); ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233263 - [Modules] Add some more fun code to my modules stress test, this time
Author: chandlerc
Date: Thu Mar 26 03:49:55 2015
New Revision: 233263
URL: http://llvm.org/viewvc/llvm-project?rev=233263view=rev
Log:
[Modules] Add some more fun code to my modules stress test, this time
templates. Turns out all of this works correctly (so far). But it should
cover more code paths and will let me test some things that don't
actually work next.
Modified:
cfe/trunk/test/Modules/Inputs/stress1/common.h
cfe/trunk/test/Modules/Inputs/stress1/merge00.h
Modified: cfe/trunk/test/Modules/Inputs/stress1/common.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/stress1/common.h?rev=233263r1=233262r2=233263view=diff
==
--- cfe/trunk/test/Modules/Inputs/stress1/common.h (original)
+++ cfe/trunk/test/Modules/Inputs/stress1/common.h Thu Mar 26 03:49:55 2015
@@ -5,6 +5,8 @@ inline char function00(char x) { return
inline short function00(short x) { return x + x; }
inline int function00(int x) { return x + x; }
+namespace N01 { struct S00; }
+
namespace N00 {
struct S00 {
char c;
@@ -24,7 +26,19 @@ struct S00 {
operator int() { return i; }
};
struct S01 {};
-struct S03 {};
+struct S02 {};
+template typename T struct S03 {
+ struct S00 : N00::S00 {};
+};
+template int I, template typename class U struct S03Uint[I]
+: Uint::S00 {
+ S03();
+ S03(int);
+ S03(short);
+ S03(char);
+ template typename V = decltype(I) S03(V);
+};
+template struct S03S03int[42] : S00 {};
}
namespace N01 {
@@ -35,6 +49,14 @@ struct S01 {};
struct S02 {};
}
+using namespace N00;
+
+template int I, template typename class U template typename V
S03Uint[I]::S03(V x) : S00(x) {}
+template int I, template typename class U S03Uint[I]::S03() : S00(I) {}
+template int I, template typename class U S03Uint[I]::S03(char x) :
S00(x) {}
+template int I, template typename class U S03Uint[I]::S03(short x) :
S00(x) {}
+template int I, template typename class U S03Uint[I]::S03(int x) :
S00(x) {}
+
#pragma weak pragma_weak00
#pragma weak pragma_weak01
#pragma weak pragma_weak02
Modified: cfe/trunk/test/Modules/Inputs/stress1/merge00.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/stress1/merge00.h?rev=233263r1=233262r2=233263view=diff
==
--- cfe/trunk/test/Modules/Inputs/stress1/merge00.h (original)
+++ cfe/trunk/test/Modules/Inputs/stress1/merge00.h Thu Mar 26 03:49:55 2015
@@ -14,7 +14,7 @@
#include m02.h
#include m03.h
-inline int g() { return N00::S00('a').method00('b') + (int)N00::S00(42) +
function00(42); }
+inline int g() { return N00::S00('a').method00('b') + (int)S00(42) +
function00(42); }
#pragma weak pragma_weak02
#pragma weak pragma_weak05
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233261 - [Modules] Make #pragma weak undeclared identifiers be tracked
Author: chandlerc
Date: Thu Mar 26 03:32:49 2015
New Revision: 233261
URL: http://llvm.org/viewvc/llvm-project?rev=233261view=rev
Log:
[Modules] Make #pragma weak undeclared identifiers be tracked
deterministically.
This fixes a latent issue where even Clang's Sema (and diagnostics) were
non-deterministic in the face of this pragma. The fix is super simple --
just use a MapVector so we track the order in which these are parsed (or
imported). Especially considering how rare they are, this seems like the
perfect tradeoff. I've also simplified the client code with judicious
use of auto and range based for loops.
I've added some pretty hilarious code to my stress test which now
survives the binary diff without issue.
Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/Sema.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/Modules/Inputs/stress1/common.h
cfe/trunk/test/Modules/Inputs/stress1/merge00.h
cfe/trunk/test/Modules/stress1.cpp
Modified: cfe/trunk/include/clang/Sema/Sema.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=233261r1=233260r2=233261view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Thu Mar 26 03:32:49 2015
@@ -592,7 +592,7 @@ public:
/// WeakUndeclaredIdentifiers - Identifiers contained in
/// \#pragma weak before declared. rare. may alias another
/// identifier, declared or undeclared
- llvm::DenseMapIdentifierInfo*,WeakInfo WeakUndeclaredIdentifiers;
+ llvm::MapVectorIdentifierInfo *, WeakInfo WeakUndeclaredIdentifiers;
/// ExtnameUndeclaredIdentifiers - Identifiers contained in
/// \#pragma redefine_extname before declared. Used in Solaris system
headers
Modified: cfe/trunk/lib/Sema/Sema.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.cpp?rev=233261r1=233260r2=233261view=diff
==
--- cfe/trunk/lib/Sema/Sema.cpp (original)
+++ cfe/trunk/lib/Sema/Sema.cpp Thu Mar 26 03:32:49 2015
@@ -524,14 +524,8 @@ void Sema::LoadExternalWeakUndeclaredIde
SmallVectorstd::pairIdentifierInfo *, WeakInfo, 4 WeakIDs;
ExternalSource-ReadWeakUndeclaredIdentifiers(WeakIDs);
- for (unsigned I = 0, N = WeakIDs.size(); I != N; ++I) {
-llvm::DenseMapIdentifierInfo*,WeakInfo::iterator Pos
- = WeakUndeclaredIdentifiers.find(WeakIDs[I].first);
-if (Pos != WeakUndeclaredIdentifiers.end())
- continue;
-
-WeakUndeclaredIdentifiers.insert(WeakIDs[I]);
- }
+ for (auto WeakID : WeakIDs)
+WeakUndeclaredIdentifiers.insert(WeakID);
}
@@ -694,16 +688,13 @@ void Sema::ActOnEndOfTranslationUnit() {
}
// Check for #pragma weak identifiers that were never declared
- // FIXME: This will cause diagnostics to be emitted in a non-determinstic
- // order! Iterating over a densemap like this is bad.
LoadExternalWeakUndeclaredIdentifiers();
- for (llvm::DenseMapIdentifierInfo*,WeakInfo::iterator
- I = WeakUndeclaredIdentifiers.begin(),
- E = WeakUndeclaredIdentifiers.end(); I != E; ++I) {
-if (I-second.getUsed()) continue;
+ for (auto WeakID : WeakUndeclaredIdentifiers) {
+if (WeakID.second.getUsed())
+ continue;
-Diag(I-second.getLocation(), diag::warn_weak_identifier_undeclared)
- I-first;
+Diag(WeakID.second.getLocation(), diag::warn_weak_identifier_undeclared)
+ WeakID.first;
}
if (LangOpts.CPlusPlus11
Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=233261r1=233260r2=233261view=diff
==
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Thu Mar 26 03:32:49 2015
@@ -5019,8 +5019,7 @@ void Sema::ProcessPragmaWeak(Scope *S, D
ND = FD;
if (ND) {
if (IdentifierInfo *Id = ND-getIdentifier()) {
-llvm::DenseMapIdentifierInfo*,WeakInfo::iterator I
- = WeakUndeclaredIdentifiers.find(Id);
+auto I = WeakUndeclaredIdentifiers.find(Id);
if (I != WeakUndeclaredIdentifiers.end()) {
WeakInfo W = I-second;
DeclApplyPragmaWeak(S, ND, W);
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233261r1=233260r2=233261view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Mar 26 03:32:49 2015
@@ -4359,15 +4359,13 @@ void ASTWriter::WriteASTCore(Sema SemaR
// entire table, since later PCH files in a PCH chain are only interested in
// the results at the end of the chain.
r233264 - [Modules] Preserve source order for the map of late parsed templates.
Author: chandlerc
Date: Thu Mar 26 04:08:15 2015
New Revision: 233264
URL: http://llvm.org/viewvc/llvm-project?rev=233264view=rev
Log:
[Modules] Preserve source order for the map of late parsed templates.
Clang was inserting these into a dense map. While it never iterated the
dense map during normal compilation, it did when emitting a module. Fix
this by using a standard MapVector to preserve the order in which we
encounter the late parsed templates.
I suspect this still isn't ideal, as we don't seem to remove things from
this map even when we mark the templates as no longer late parsed. But
I don't know enough about this particular extension to craft a nice,
subtle test case covering this. I've managed to get the stress test to
at least do some late parsing and demonstrate the core problem here.
This patch fixes the test and provides deterministic behavior which is
a strict improvement over the prior state.
I've cleaned up some of the code here as well to be explicit about
inserting when that is what is actually going on.
Modified:
cfe/trunk/include/clang/Sema/ExternalSemaSource.h
cfe/trunk/include/clang/Sema/MultiplexExternalSemaSource.h
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/include/clang/Serialization/ASTReader.h
cfe/trunk/lib/Sema/MultiplexExternalSemaSource.cpp
cfe/trunk/lib/Sema/SemaTemplate.cpp
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/Modules/stress1.cpp
Modified: cfe/trunk/include/clang/Sema/ExternalSemaSource.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/ExternalSemaSource.h?rev=233264r1=233263r2=233264view=diff
==
--- cfe/trunk/include/clang/Sema/ExternalSemaSource.h (original)
+++ cfe/trunk/include/clang/Sema/ExternalSemaSource.h Thu Mar 26 04:08:15 2015
@@ -182,7 +182,7 @@ public:
/// external source should take care not to introduce the same map entries
/// repeatedly.
virtual void ReadLateParsedTemplates(
- llvm::DenseMapconst FunctionDecl *, LateParsedTemplate * LPTMap) {}
+ llvm::MapVectorconst FunctionDecl *, LateParsedTemplate * LPTMap) {}
/// \copydoc Sema::CorrectTypo
/// \note LookupKind must correspond to a valid Sema::LookupNameKind
Modified: cfe/trunk/include/clang/Sema/MultiplexExternalSemaSource.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/MultiplexExternalSemaSource.h?rev=233264r1=233263r2=233264view=diff
==
--- cfe/trunk/include/clang/Sema/MultiplexExternalSemaSource.h (original)
+++ cfe/trunk/include/clang/Sema/MultiplexExternalSemaSource.h Thu Mar 26
04:08:15 2015
@@ -331,8 +331,8 @@ public:
/// external source should take care not to introduce the same map entries
/// repeatedly.
void ReadLateParsedTemplates(
- llvm::DenseMapconst FunctionDecl *,
-LateParsedTemplate * LPTMap)
override;
+ llvm::MapVectorconst FunctionDecl *, LateParsedTemplate * LPTMap)
+ override;
/// \copydoc ExternalSemaSource::CorrectTypo
/// \note Returns the first nonempty correction.
Modified: cfe/trunk/include/clang/Sema/Sema.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=233264r1=233263r2=233264view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Thu Mar 26 04:08:15 2015
@@ -455,8 +455,8 @@ public:
SmallVectorstd::pairCXXMethodDecl*, const FunctionProtoType*, 2
DelayedDefaultedMemberExceptionSpecs;
- typedef llvm::DenseMapconst FunctionDecl *, LateParsedTemplate *
- LateParsedTemplateMapT;
+ typedef llvm::MapVectorconst FunctionDecl *, LateParsedTemplate *
+ LateParsedTemplateMapT;
LateParsedTemplateMapT LateParsedTemplateMap;
/// \brief Callback to the parser to parse templated functions when needed.
Modified: cfe/trunk/include/clang/Serialization/ASTReader.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=233264r1=233263r2=233264view=diff
==
--- cfe/trunk/include/clang/Serialization/ASTReader.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTReader.h Thu Mar 26 04:08:15 2015
@@ -1832,8 +1832,8 @@ public:
SourceLocation Pending)
override;
void ReadLateParsedTemplates(
- llvm::DenseMapconst FunctionDecl *,
-LateParsedTemplate * LPTMap)
override;
+ llvm::MapVectorconst FunctionDecl *, LateParsedTemplate * LPTMap)
+ override;
/// \brief Load a selector from disk, registering its ID if it exists.
void LoadSelector(Selector
Re: r233249 - [Modules] A second attempt at writing out on-disk hash tables for the
Just FYI in case others see this:
On Wed, Mar 25, 2015 at 8:11 PM, Chandler Carruth chandl...@gmail.com
wrote:
+assert(ConstructorNameSet.empty() Failed to find all of the
visible
+ constructors by walking all the
+ lexical members of the
context.);
I've got at least one case where this assert trips. It seems really obscure
and only the modules selfhost bot has seen it so I've not reverted, but if
this is causing anyone trouble, feel free.
I've got it reproduced technically, but still digging into exactly what the
right fix will be. The case looks like:
(gdb) p ConstructorNameSet.Vector.size()
$1 = 1
(gdb) p DC-dumpLookups()
StoredDeclsMap CXXRecord 0x7fffca58e458 ''
|-DeclarationName '__count'
| `-Field 0x7fffc47da8a0 '__count' 'int'
|-DeclarationName ''
| |-CXXConstructor 0x7fffca58e8b0 '' 'void (void) noexcept'
| |-CXXConstructor 0x7fffca58e998 '' 'void (const __mbstate_t )'
| `-CXXConstructor 0x7fffca58eb88 '' 'void (__mbstate_t ) noexcept'
|-DeclarationName '__value'
| `-Field 0x7fffc47da900 '__value' 'union (anonymous union at
/usr/include/wchar.h:85:3)':'union __mbstate_t::(anonymous at
/usr/include/wchar.h:85:3)'
|-DeclarationName '~'
| `-CXXDestructor 0x7fffc47da7b0 '~' 'void (void)'
`-undeserialized lookups
$2 = void
(gdb) p clang::Decl::castFromDeclContext(DC)-dump()
CXXRecordDecl 0x7fffca58e458 /usr/include/wchar.h:82:9, line:94:1
line:82:9 in __usr_include.wchar struct definition
|-CXXRecordDecl 0x7fffc47da950 line:85:3, line:93:3 line:85:3 in
__usr_include.wchar union definition
| |-FieldDecl 0x7fffc47db788 built-in:104:23,
/usr/include/wchar.h:88:19 col:19 in __usr_include.wchar __wch 'unsigned
int'
| `-FieldDecl 0x7fffc47db7e8 line:92:5, col:18 col:10 in
__usr_include.wchar __wchb 'char [4]'
|-FieldDecl 0x7fffc47da8a0 line:84:3, col:7 col:7 in __usr_include.wchar
referenced __count 'int'
|-FieldDecl 0x7fffc47da900 line:85:3, line:93:5 col:5 in
__usr_include.wchar referenced __value 'union (anonymous union at
/usr/include/wchar.h:85:3)':'union __mbstate_t::(anonymous at
/usr/include/wchar.h:85:3)'
`-CXXDestructorDecl 0x7fffc47da7b0 line:82:9 col:9 implicit ~ 'void
(void)' inline noexcept-unevaluated 0x7fffc47da7b0
$3 = void
And the type in question is roughly:
typedef struct {
int __count;
union {
wint_t __wch;
char __wchb[4];
} __value;
} __mbstate_t;
So far, naive attempts to produce a more targeted test case have failed.
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r233249 - [Modules] A second attempt at writing out on-disk hash tables for the
On Thu, Mar 26, 2015 at 1:12 PM, Richard Smith rich...@metafoo.co.uk wrote: So, the problem seems to be that the class has implicit special members that are not in the list of lexical declarations of the class but are in the visible lookup results. In order for this to happen, you might need to have two definitions of a class that get merged together, where the implicit special members are only present in the definition that we demoted to a declaration. Wouldn't that be a bug? It certainly looks like a bug -- the lookup table has named decl in it for the constructor name that is a wild pointer. Perhaps the simplest thing to do would be to add the name of the current class to Names before performing the lexical walk of the class, if it's in the list of constructor names. That would also let you avoid the lexical walk entirely if the only constructor or conversion name is for the class's own constructor(s) (that is, if the class has no inheriting constructors or conversion functions, but does have constructors, which is likely to be a very common case). ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233339 - [Modules] Sort the file IDs prior to building the flattened array of
Author: chandlerc
Date: Thu Mar 26 19:31:20 2015
New Revision: 29
URL: http://llvm.org/viewvc/llvm-project?rev=29view=rev
Log:
[Modules] Sort the file IDs prior to building the flattened array of
DeclIDs so that in addition to be grouped by file, the order of these
groups is stable.
Found by inspection, no test case. Not sure this can be observed without
a randomized seed for the hash table, but we shouldn't be relying on the
hash table layout under any circumstances.
Modified:
cfe/trunk/lib/Serialization/ASTWriter.cpp
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=29r1=28r2=29view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Mar 26 19:31:20 2015
@@ -2857,15 +2857,18 @@ void ASTWriter::WriteFileDeclIDsMap() {
using namespace llvm;
RecordData Record;
+ SmallVectorstd::pairFileID, DeclIDInFileInfo *, 64 SortedFileDeclIDs(
+ FileDeclIDs.begin(), FileDeclIDs.end());
+ std::sort(SortedFileDeclIDs.begin(), SortedFileDeclIDs.end(),
+llvm::less_first());
+
// Join the vectors of DeclIDs from all files.
- SmallVectorDeclID, 256 FileSortedIDs;
- for (FileDeclIDsTy::iterator
- FI = FileDeclIDs.begin(), FE = FileDeclIDs.end(); FI != FE; ++FI) {
-DeclIDInFileInfo Info = *FI-second;
-Info.FirstDeclIndex = FileSortedIDs.size();
-for (LocDeclIDsTy::iterator
- DI = Info.DeclIDs.begin(), DE = Info.DeclIDs.end(); DI != DE; ++DI)
- FileSortedIDs.push_back(DI-second);
+ SmallVectorDeclID, 256 FileGroupedDeclIDs;
+ for (auto FileDeclEntry : SortedFileDeclIDs) {
+DeclIDInFileInfo Info = *FileDeclEntry.second;
+Info.FirstDeclIndex = FileGroupedDeclIDs.size();
+for (auto LocDeclEntry : Info.DeclIDs)
+ FileGroupedDeclIDs.push_back(LocDeclEntry.second);
}
BitCodeAbbrev *Abbrev = new BitCodeAbbrev();
@@ -2874,8 +2877,8 @@ void ASTWriter::WriteFileDeclIDsMap() {
Abbrev-Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));
unsigned AbbrevCode = Stream.EmitAbbrev(Abbrev);
Record.push_back(FILE_SORTED_DECLS);
- Record.push_back(FileSortedIDs.size());
- Stream.EmitRecordWithBlob(AbbrevCode, Record, data(FileSortedIDs));
+ Record.push_back(FileGroupedDeclIDs.size());
+ Stream.EmitRecordWithBlob(AbbrevCode, Record, data(FileGroupedDeclIDs));
}
void ASTWriter::WriteComments() {
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233342 - [Modules] Make our on-disk hash table of selector IDs be built in
Author: chandlerc
Date: Thu Mar 26 19:47:43 2015
New Revision: 233342
URL: http://llvm.org/viewvc/llvm-project?rev=233342view=rev
Log:
[Modules] Make our on-disk hash table of selector IDs be built in
a deterministic order.
This uses a MapVector to track the insertion order of selectors.
Found by inspection.
Modified:
cfe/trunk/include/clang/Serialization/ASTWriter.h
cfe/trunk/lib/Serialization/ASTWriter.cpp
Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTWriter.h?rev=233342r1=233341r2=233342view=diff
==
--- cfe/trunk/include/clang/Serialization/ASTWriter.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTWriter.h Thu Mar 26 19:47:43 2015
@@ -276,7 +276,7 @@ private:
serialization::SelectorID NextSelectorID;
/// \brief Map that provides the ID numbers of each Selector.
- llvm::DenseMapSelector, serialization::SelectorID SelectorIDs;
+ llvm::MapVectorSelector, serialization::SelectorID SelectorIDs;
/// \brief Offset of each selector within the method pool/selector
/// table, indexed by the Selector ID (-1).
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233342r1=233341r2=233342view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Mar 26 19:47:43 2015
@@ -3029,13 +3029,12 @@ void ASTWriter::WriteSelectors(Sema Sem
// Create the on-disk hash table representation. We walk through every
// selector we've seen and look it up in the method pool.
SelectorOffsets.resize(NextSelectorID - FirstSelectorID);
-for (llvm::DenseMapSelector, SelectorID::iterator
- I = SelectorIDs.begin(), E = SelectorIDs.end();
- I != E; ++I) {
- Selector S = I-first;
+for (auto SelectorAndID : SelectorIDs) {
+ Selector S = SelectorAndID.first;
+ SelectorID ID = SelectorAndID.second;
Sema::GlobalMethodPool::iterator F = SemaRef.MethodPool.find(S);
ASTMethodPoolTrait::data_type Data = {
-I-second,
+ID,
ObjCMethodList(),
ObjCMethodList()
};
@@ -3045,7 +3044,7 @@ void ASTWriter::WriteSelectors(Sema Sem
}
// Only write this selector if it's not in an existing AST or something
// changed.
- if (Chain I-second FirstSelectorID) {
+ if (Chain ID FirstSelectorID) {
// Selector already exists. Did it change?
bool changed = false;
for (ObjCMethodList *M = Data.Instance;
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233343 - [Modules] Make Sema's map of referenced selectors have a deterministic
Author: chandlerc
Date: Thu Mar 26 19:55:05 2015
New Revision: 233343
URL: http://llvm.org/viewvc/llvm-project?rev=233343view=rev
Log:
[Modules] Make Sema's map of referenced selectors have a deterministic
order based on order of insertion.
This should cause both our warnings about these and the modules
serialization to be deterministic as a consequence.
Found by inspection.
Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaDeclObjC.cpp
cfe/trunk/lib/Sema/SemaExprObjC.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
Modified: cfe/trunk/include/clang/Sema/Sema.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=233343r1=233342r2=233343view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Thu Mar 26 19:55:05 2015
@@ -899,7 +899,7 @@ public:
/// Method selectors used in a \@selector expression. Used for implementation
/// of -Wselector.
- llvm::DenseMapSelector, SourceLocation ReferencedSelectors;
+ llvm::MapVectorSelector, SourceLocation ReferencedSelectors;
/// Kinds of C++ special members.
enum CXXSpecialMember {
Modified: cfe/trunk/lib/Sema/SemaDeclObjC.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclObjC.cpp?rev=233343r1=233342r2=233343view=diff
==
--- cfe/trunk/lib/Sema/SemaDeclObjC.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclObjC.cpp Thu Mar 26 19:55:05 2015
@@ -3490,12 +3490,11 @@ void Sema::DiagnoseUseOfUnimplementedSel
if (ReferencedSelectors.empty() ||
!Context.AnyObjCImplementation())
return;
- for (llvm::DenseMapSelector, SourceLocation::iterator S =
-ReferencedSelectors.begin(),
- E = ReferencedSelectors.end(); S != E; ++S) {
-Selector Sel = (*S).first;
+ for (auto SelectorAndLocation : ReferencedSelectors) {
+Selector Sel = SelectorAndLocation.first;
+SourceLocation Loc = SelectorAndLocation.second;
if (!LookupImplementedMethodInGlobalPool(Sel))
- Diag((*S).second, diag::warn_unimplemented_selector) Sel;
+ Diag(Loc, diag::warn_unimplemented_selector) Sel;
}
return;
}
Modified: cfe/trunk/lib/Sema/SemaExprObjC.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprObjC.cpp?rev=233343r1=233342r2=233343view=diff
==
--- cfe/trunk/lib/Sema/SemaExprObjC.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExprObjC.cpp Thu Mar 26 19:55:05 2015
@@ -1061,15 +1061,11 @@ ExprResult Sema::ParseObjCSelectorExpres
} else
DiagnoseMismatchedSelectors(*this, AtLoc, Method, LParenLoc, RParenLoc,
WarnMultipleSelectors);
-
+
if (Method
Method-getImplementationControl() != ObjCMethodDecl::Optional
- !getSourceManager().isInSystemHeader(Method-getLocation())) {
-llvm::DenseMapSelector, SourceLocation::iterator Pos
- = ReferencedSelectors.find(Sel);
-if (Pos == ReferencedSelectors.end())
- ReferencedSelectors.insert(std::make_pair(Sel, AtLoc));
- }
+ !getSourceManager().isInSystemHeader(Method-getLocation()))
+ReferencedSelectors.insert(std::make_pair(Sel, AtLoc));
// In ARC, forbid the user from using @selector for
// retain/release/autorelease/dealloc/retainCount.
@@ -2743,8 +2739,7 @@ static void RemoveSelectorFromWarningCac
dyn_castObjCSelectorExpr(Arg-IgnoreParenCasts())) {
Selector Sel = OSE-getSelector();
SourceLocation Loc = OSE-getAtLoc();
-llvm::DenseMapSelector, SourceLocation::iterator Pos
-= S.ReferencedSelectors.find(Sel);
+auto Pos = S.ReferencedSelectors.find(Sel);
if (Pos != S.ReferencedSelectors.end() Pos-second == Loc)
S.ReferencedSelectors.erase(Pos);
}
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233343r1=233342r2=233343view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Mar 26 19:55:05 2015
@@ -3122,11 +3122,9 @@ void ASTWriter::WriteReferencedSelectors
// Note: this writes out all references even for a dependent AST. But it is
// very tricky to fix, and given that @selector shouldn't really appear in
// headers, probably not worth it. It's not a correctness issue.
- for (DenseMapSelector, SourceLocation::iterator S =
- SemaRef.ReferencedSelectors.begin(),
- E = SemaRef.ReferencedSelectors.end(); S != E; ++S) {
-Selector Sel = (*S).first;
-SourceLocation Loc = (*S).second;
+ for (auto SelectorAndLocation : SemaRef.ReferencedSelectors) {
+Selector Sel = SelectorAndLocation.first;
+SourceLocation Loc =
r233331 - [Modules] Delete stale, pointless code. All tests still pass with this
Author: chandlerc
Date: Thu Mar 26 18:45:40 2015
New Revision: 21
URL: http://llvm.org/viewvc/llvm-project?rev=21view=rev
Log:
[Modules] Delete stale, pointless code. All tests still pass with this
logic removed.
This logic was both inserting all builtins into the identifier table and
ensuring they would get serialized. The first happens unconditionally
now, and we always write out the entire identifier table. This code can
simply go away.
Modified:
cfe/trunk/lib/Serialization/ASTWriter.cpp
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=21r1=20r2=21view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Mar 26 18:45:40 2015
@@ -4335,21 +4335,6 @@ void ASTWriter::WriteASTCore(Sema SemaR
if (Context.ExternCContext)
DeclIDs[Context.ExternCContext] = PREDEF_DECL_EXTERN_C_CONTEXT_ID;
- if (!Chain) {
-// Make sure that we emit IdentifierInfos (and any attached
-// declarations) for builtins. We don't need to do this when we're
-// emitting chained PCH files, because all of the builtins will be
-// in the original PCH file.
-// FIXME: Modules won't like this at all.
-IdentifierTable Table = PP.getIdentifierTable();
-SmallVectorconst char *, 32 BuiltinNames;
-if (!Context.getLangOpts().NoBuiltin) {
- Context.BuiltinInfo.GetBuiltinNames(BuiltinNames);
-}
-for (unsigned I = 0, N = BuiltinNames.size(); I != N; ++I)
- getIdentifierRef(Table.get(BuiltinNames[I]));
- }
-
// Build a record containing all of the tentative definitions in this file,
in
// TentativeDefinitions order. Generally, this record will be empty for
// headers.
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233332 - [Modules] Make the AST serialization always use lexicographic order when
Author: chandlerc
Date: Thu Mar 26 18:54:15 2015
New Revision: 22
URL: http://llvm.org/viewvc/llvm-project?rev=22view=rev
Log:
[Modules] Make the AST serialization always use lexicographic order when
traversing the identifier table.
No easy test case as this table is somewhere between hard and impossible
to observe as non-deterministically ordered. The table is a hash table
but we hash the string contents and never remove entries from the table
so the growth pattern, etc, is all completely fixed. However, relying on
the hash function being deterministic is specifically against the
long-term direction of LLVM's hashing datastructures, which are intended
to provide *no* ordering guarantees. As such, this defends against these
things by sorting the identifiers. Sorting identifiers right before we
emit them to a serialized form seems a low cost for predictability here.
Modified:
cfe/trunk/include/clang/Basic/IdentifierTable.h
cfe/trunk/lib/Serialization/ASTWriter.cpp
Modified: cfe/trunk/include/clang/Basic/IdentifierTable.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/IdentifierTable.h?rev=22r1=21r2=22view=diff
==
--- cfe/trunk/include/clang/Basic/IdentifierTable.h (original)
+++ cfe/trunk/include/clang/Basic/IdentifierTable.h Thu Mar 26 18:54:15 2015
@@ -308,7 +308,12 @@ public:
else
RecomputeNeedsHandleIdentifier();
}
-
+
+ /// \brief Provide less than operator for lexicographical sorting.
+ bool operator(const IdentifierInfo RHS) const {
+return getName() RHS.getName();
+ }
+
private:
/// The Preprocessor::HandleIdentifier does several special (but rare)
/// things to identifiers of various sorts. For example, it changes the
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=22r1=21r2=22view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Mar 26 18:54:15 2015
@@ -3504,10 +3504,16 @@ void ASTWriter::WriteIdentifierTable(Pre
// table to enable checking of the predefines buffer in the case
// where the user adds new macro definitions when building the AST
// file.
+SmallVectorconst IdentifierInfo *, 128 IIs;
for (IdentifierTable::iterator ID = PP.getIdentifierTable().begin(),
IDEnd = PP.getIdentifierTable().end();
ID != IDEnd; ++ID)
- getIdentifierRef(ID-second);
+ IIs.push_back(ID-second);
+// Sort the identifiers lexicographically before getting them references so
+// that their order is stable.
+std::sort(IIs.begin(), IIs.end(), llvm::less_ptrIdentifierInfo());
+for (const IdentifierInfo *II : IIs)
+ getIdentifierRef(II);
// Create the on-disk hash table representation. We only store offsets
// for identifiers that appear here for the first time.
@@ -4504,15 +4510,17 @@ void ASTWriter::WriteASTCore(Sema SemaR
// Make sure all decls associated with an identifier are registered for
// serialization.
- llvm::SmallVectorconst IdentifierInfo*, 256 IIsToVisit;
+ llvm::SmallVectorconst IdentifierInfo*, 256 IIs;
for (IdentifierTable::iterator ID = PP.getIdentifierTable().begin(),
IDEnd = PP.getIdentifierTable().end();
ID != IDEnd; ++ID) {
const IdentifierInfo *II = ID-second;
if (!Chain || !II-isFromAST() || II-hasChangedSinceDeserialization())
- IIsToVisit.push_back(II);
+ IIs.push_back(II);
}
- for (const IdentifierInfo *II : IIsToVisit) {
+ // Sort the identifiers to visit based on their name.
+ std::sort(IIs.begin(), IIs.end(), llvm::less_ptrIdentifierInfo());
+ for (const IdentifierInfo *II : IIs) {
for (IdentifierResolver::iterator D = SemaRef.IdResolver.begin(II),
DEnd = SemaRef.IdResolver.end();
D != DEnd; ++D) {
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233333 - [Modules] Fix an obvious lack of deterministic ordering when processing
Author: chandlerc Date: Thu Mar 26 18:58:11 2015 New Revision: 23 URL: http://llvm.org/viewvc/llvm-project?rev=23view=rev Log: [Modules] Fix an obvious lack of deterministic ordering when processing rewritten decls for Objective-C modules. Found by inspection and completely obvious, so no test case. Many of the remaining determinism fixes won't have precise test cases at this point, but these are the kinds of things we wouldn't ask for a specific test of during code review but ask authors to fix. The functionality isn't changing, and should (he he!) already be tested. Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTWriter.h?rev=23r1=22r2=23view=diff == --- cfe/trunk/include/clang/Serialization/ASTWriter.h (original) +++ cfe/trunk/include/clang/Serialization/ASTWriter.h Thu Mar 26 18:58:11 2015 @@ -358,7 +358,7 @@ private: /// coming from another AST file. SmallVectorconst Decl *, 16 UpdatingVisibleDecls; - typedef llvm::SmallPtrSetconst Decl *, 16 DeclsToRewriteTy; + typedef llvm::SmallSetVectorconst Decl *, 16 DeclsToRewriteTy; /// \brief Decls that will be replaced in the current dependent AST file. DeclsToRewriteTy DeclsToRewrite; ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233334 - [Modules] Clean up some code that was manually replicating what
Author: chandlerc Date: Thu Mar 26 18:59:47 2015 New Revision: 24 URL: http://llvm.org/viewvc/llvm-project?rev=24view=rev Log: [Modules] Clean up some code that was manually replicating what SmallSetVector provides directly. Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTWriter.h?rev=24r1=23r2=24view=diff == --- cfe/trunk/include/clang/Serialization/ASTWriter.h (original) +++ cfe/trunk/include/clang/Serialization/ASTWriter.h Thu Mar 26 18:59:47 2015 @@ -387,8 +387,7 @@ private: /// \brief The set of declarations that may have redeclaration chains that /// need to be serialized. - llvm::SetVectorDecl *, SmallVectorDecl *, 4, - llvm::SmallPtrSetDecl *, 4 Redeclarations; + llvm::SmallSetVectorDecl *, 4 Redeclarations; /// \brief Statements that we've encountered while serializing a /// declaration or type. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233335 - [Modules] Fix another pointer keyed set that we iterate over while
Author: chandlerc Date: Thu Mar 26 19:01:44 2015 New Revision: 25 URL: http://llvm.org/viewvc/llvm-project?rev=25view=rev Log: [Modules] Fix another pointer keyed set that we iterate over while writing a module to be a set-vector to preserve insertion order. No test case, found by inspection. Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTWriter.h?rev=25r1=24r2=25view=diff == --- cfe/trunk/include/clang/Serialization/ASTWriter.h (original) +++ cfe/trunk/include/clang/Serialization/ASTWriter.h Thu Mar 26 19:01:44 2015 @@ -352,7 +352,7 @@ private: /// if its primary namespace comes from the chain. If it does, we add the /// primary to this set, so that we can write out lexical content updates for /// it. - llvm::SmallPtrSetconst DeclContext *, 16 UpdatedDeclContexts; + llvm::SmallSetVectorconst DeclContext *, 16 UpdatedDeclContexts; /// \brief Keeps track of visible decls that were added in DeclContexts /// coming from another AST file. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r233249 - [Modules] A second attempt at writing out on-disk hash tables for the
On Thu, Mar 26, 2015 at 1:39 PM, Chandler Carruth chandl...@gmail.com wrote: On Thu, Mar 26, 2015 at 1:12 PM, Richard Smith rich...@metafoo.co.uk wrote: So, the problem seems to be that the class has implicit special members that are not in the list of lexical declarations of the class but are in the visible lookup results. In order for this to happen, you might need to have two definitions of a class that get merged together, where the implicit special members are only present in the definition that we demoted to a declaration. Wouldn't that be a bug? It certainly looks like a bug -- the lookup table has named decl in it for the constructor name that is a wild pointer. Just to relay on the mailing list, I chatted with Richard and he explained that these decls are in some other lexical context on the redecl chain, and thus correct to find via name lookup but incorrect to merge into this lexical context. I've implemented the fix of special casing the current context's implicit constructor name (should it exist) since those can come from other contexts in the redecl chain and those are the only constructor or conversion function names which can come from other contexts in the redecl chain due to the ODR. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233325 - [Modules] Fix tiny bug where we failed to get the canonical decl when
Author: chandlerc Date: Thu Mar 26 17:22:22 2015 New Revision: 233325 URL: http://llvm.org/viewvc/llvm-project?rev=233325view=rev Log: [Modules] Fix tiny bug where we failed to get the canonical decl when deserializing an inherited constructor. This is the exact same logic we use when deserializing method overrides for the same reason: the canonical decl may end up pinned to a different decl when we are improting modules, we need to re-pin to the canonical one during reading. My test case for this will come in a subsequent commit. I was trying to test a more tricky bug fix and the test case happened to tickle this bug as well. Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=233325r1=233324r2=233325view=diff == --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original) +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Thu Mar 26 17:22:22 2015 @@ -1628,7 +1628,7 @@ void ASTDeclReader::VisitCXXConstructorD if (auto *CD = ReadDeclAsCXXConstructorDecl(Record, Idx)) if (D-isCanonicalDecl()) - D-setInheritedConstructor(CD); + D-setInheritedConstructor(CD-getCanonicalDecl()); D-IsExplicitSpecified = Record[Idx++]; } ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233327 - [Modules] Fix a sneaky bug in r233249 where we would look for implicit
Author: chandlerc
Date: Thu Mar 26 17:27:09 2015
New Revision: 233327
URL: http://llvm.org/viewvc/llvm-project?rev=233327view=rev
Log:
[Modules] Fix a sneaky bug in r233249 where we would look for implicit
constructors in the current lexical context even though name lookup
found them via some other context merged into the redecl chain.
This can only happen for implicit constructors which can only have the
name of the type of the current context, so we can fix this by simply
*always* merging those names first. This also has the advantage of
removing the walk of the current lexical context from the common case
when this is the only constructor name we need to deal with (implicit or
otherwise).
I've enhanced the tests to cover this case (and uncovered an unrelated
bug which I fixed in r233325).
Modified:
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/Modules/Inputs/stress1/m01.h
cfe/trunk/test/Modules/Inputs/stress1/merge00.h
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233327r1=233326r2=233327view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Mar 26 17:27:09 2015
@@ -3786,34 +3786,48 @@ ASTWriter::GenerateNameLookupTable(const
// Sort the names into a stable order.
std::sort(Names.begin(), Names.end());
- if (isaCXXRecordDecl(DC)
- (!ConstructorNameSet.empty() || !ConversionNameSet.empty())) {
+ if (auto *D = dyn_castCXXRecordDecl(DC)) {
// We need to establish an ordering of constructor and conversion function
-// names, and they don't have an intrinsic ordering. So when we have these,
-// we walk all the names in the decl and add the constructors and
-// conversion functions which are visible in the order they lexically occur
-// within the context.
-for (Decl *ChildD : DC-decls())
- if (auto *ChildND = dyn_castNamedDecl(ChildD)) {
-auto Name = ChildND-getDeclName();
-switch (Name.getNameKind()) {
-default:
- continue;
-
-case DeclarationName::CXXConstructorName:
- if (ConstructorNameSet.erase(Name))
-Names.push_back(Name);
- break;
-
-case DeclarationName::CXXConversionFunctionName:
- if (ConversionNameSet.erase(Name))
-Names.push_back(Name);
- break;
-}
+// names, and they don't have an intrinsic ordering.
+
+// First we try the easy case by forming the current context's constructor
+// name and adding that name first. This is a very useful optimization to
+// avoid walking the lexical declarations in many cases, and it also
+// handles the only case where a constructor name can come from some other
+// lexical context -- when that name is an implicit constructor merged from
+// another declaration in the redecl chain. Any non-implicit constructor or
+// conversion function which doesn't occur in all the lexical contexts
+// would be an ODR violation.
+auto ImplicitCtorName = Context-DeclarationNames.getCXXConstructorName(
+Context-getCanonicalType(Context-getRecordType(D)));
+if (ConstructorNameSet.erase(ImplicitCtorName))
+ Names.push_back(ImplicitCtorName);
+
+// If we still have constructors or conversion functions, we walk all the
+// names in the decl and add the constructors and conversion functions
+// which are visible in the order they lexically occur within the context.
+if (!ConstructorNameSet.empty() || !ConversionNameSet.empty())
+ for (Decl *ChildD : castCXXRecordDecl(DC)-decls())
+if (auto *ChildND = dyn_castNamedDecl(ChildD)) {
+ auto Name = ChildND-getDeclName();
+ switch (Name.getNameKind()) {
+ default:
+continue;
-if (ConstructorNameSet.empty() ConversionNameSet.empty())
- break;
- }
+ case DeclarationName::CXXConstructorName:
+if (ConstructorNameSet.erase(Name))
+ Names.push_back(Name);
+break;
+
+ case DeclarationName::CXXConversionFunctionName:
+if (ConversionNameSet.erase(Name))
+ Names.push_back(Name);
+break;
+ }
+
+ if (ConstructorNameSet.empty() ConversionNameSet.empty())
+break;
+}
assert(ConstructorNameSet.empty() Failed to find all of the visible
constructors by walking all the
Modified: cfe/trunk/test/Modules/Inputs/stress1/m01.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/stress1/m01.h?rev=233327r1=233326r2=233327view=diff
==
--- cfe/trunk/test/Modules/Inputs/stress1/m01.h (original)
+++
r233348 - [Modules] When walking the lookup results in a namespace, sort them by
Author: chandlerc
Date: Thu Mar 26 20:48:11 2015
New Revision: 233348
URL: http://llvm.org/viewvc/llvm-project?rev=233348view=rev
Log:
[Modules] When walking the lookup results in a namespace, sort them by
declaration name so that we mark declarations for emission in
a deterministic order (and in turn give them deterministic IDs).
This is the last for loop or data structure I can find by inspection of
the AST writer which doesn't use a deterministic order.
Found by inspection, no test case.
Modified:
cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
Modified: cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriterDecl.cpp?rev=233348r1=233347r2=233348view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriterDecl.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriterDecl.cpp Thu Mar 26 20:48:11 2015
@@ -980,15 +980,29 @@ void ASTDeclWriter::VisitNamespaceDecl(N
NamespaceDecl *NS = D-getOriginalNamespace();
Writer.UpdatedDeclContexts.insert(NS);
-// Make sure all visible decls are written. They will be recorded later.
-if (StoredDeclsMap *Map = NS-buildLookup()) {
- for (StoredDeclsMap::iterator D = Map-begin(), DEnd = Map-end();
- D != DEnd; ++D) {
-DeclContext::lookup_result R = D-second.getLookupResult();
-for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E;
- ++I)
- Writer.GetDeclRef(*I);
- }
+// Make sure all visible decls are written. They will be recorded later. We
+// do this using a side data structure so we can sort the names into
+// a deterministic order.
+StoredDeclsMap *Map = NS-buildLookup();
+SmallVectorstd::pairDeclarationName, DeclContext::lookup_result, 16
+LookupResults;
+LookupResults.reserve(Map-size());
+for (auto Entry : *Map)
+ LookupResults.push_back(
+ std::make_pair(Entry.first, Entry.second.getLookupResult()));
+
+std::sort(LookupResults.begin(), LookupResults.end(), llvm::less_first());
+for (auto NameAndResult : LookupResults) {
+ DeclarationName Name = NameAndResult.first;
+ (void)Name;
+ assert(Name.getNameKind() != DeclarationName::CXXConstructorName
+ Cannot have a constructor name in a namespace!);
+ assert(Name.getNameKind() != DeclarationName::CXXConversionFunctionName
+ Cannot have a conversion function name in a namespace!);
+
+ DeclContext::lookup_result Result = NameAndResult.second;
+ for (NamedDecl *ND : Result)
+Writer.GetDeclRef(ND);
}
}
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r220493 - Add a signature to AST files to verify that they haven't changed
FYI, as of r233348, ever source of non-deterministic output I can find by inspection and some targeted stress testing on the C++ side is fixed. I suspect we're pretty clase. Ben, if you want to add test cases for objc stuff, that'd be awesome. I just fixed it all by inspection. I think that what we should do is add a (potentially optional and only produced by an asserts build of Clang) record to the serialized AST which is a digest (MD5 or some such) of the entire AST up to that record. We could add this to the low layer of the AST writing so it would be easy to plumb through. This would make an actually *good* signature record and we could check it to ensure we don't waste time trying to debug failures if non-deterministic behavior creeps in again. Ben, could you look at making that change? I think that would be a *much* better design for the signature than a random number, and would serve the exact same purpose. On Tue, Mar 24, 2015 at 2:21 PM, Chandler Carruth chandl...@google.com wrote: On Mon, Mar 23, 2015 at 10:37 PM, Ben Langmuir blangm...@apple.com wrote: On Mar 23, 2015, at 10:21 PM, Chandler Carruth chandl...@google.com wrote: On Mon, Mar 23, 2015 at 9:54 PM, Ben Langmuir blangm...@apple.com wrote: On Mar 23, 2015, at 5:38 PM, Chandler Carruth chandl...@google.com wrote: Digging this thread back up, I'm starting to work on deterministic output of modules. This is *really* important. Unfortunately, it's almost impossible to make progress as long as this code is actually firing because it kind of makes things definitively non-deterministic. ;] Ben, do you have any concerns about only emitting the signature record for implicit modules? This would omit it for explicit modules (where I'm working), PCH files, etc. Based on your commit message and explanation, it doesn't really make sense outside of implicit modules AFAICT, and we're hoping it will go away completely. SGTM although I would prefer we continue to check the /expected/ signature when an AST file imports an implicit module (and in particular when a PCH imports an implicit module). Thanks for working on this! I'm not planning to change the checking code path at all. All the tests pass if I just restrict the emission to be when generating implicit modules Perfect, that’s what I was hoping for. Sorry I didn’t phrase it clearly. This is in as part of r233115 with the minimal test case for deterministic output. More to come. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r233172 - Revert [Modules] When writing out the on-disk hash table for the decl context lookup tables, we need to establish a stable ordering for constructing the hash table. This is trickier tha
Sorry, yes, the correct fix was to disable the tests.
I tried to do that originally, but apparently didn't disable enough
(despite running the tests many times).
On Wed, Mar 25, 2015 at 12:02 AM, Daniel Jasper djas...@google.com wrote:
The problem is that the test is for testing non-determinism. And obviously
there might just be instances where the test passes out of luck.
I have temporarily disabled one more of those tests in r233173, which
should hopefully appease all the bots for now.
On Wed, Mar 25, 2015 at 7:35 AM, Justin Bogner m...@justinbogner.com
wrote:
Rafael Espindola rafael.espind...@gmail.com writes:
Author: rafael
Date: Tue Mar 24 23:43:15 2015
New Revision: 233172
URL: http://llvm.org/viewvc/llvm-project?rev=233172view=rev
Log:
Revert [Modules] When writing out the on-disk hash table for the decl
context lookup tables, we need to establish a stable ordering for
constructing the hash table. This is trickier than it might seem.
This reverts commit r233156. It broke the bots.
Which bots did it break? Lots of bots (and my local check-clang) went
red from the revert, because the tests in r233162 depend on it. I can't
actually find any bots that look like r233156 broke (at least that were
still broken after r233162+r233163), but I'm probably not looking in the
right place.
Modified:
cfe/trunk/lib/Serialization/ASTWriter.cpp
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233172r1=233171r2=233172view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Tue Mar 24 23:43:15 2015
@@ -3761,34 +3761,15 @@ ASTWriter::GenerateNameLookupTable(const
!DC-HasLazyExternalLexicalLookups
must call buildLookups first);
- // Create the on-disk hash table representation.
llvm::OnDiskChainedHashTableGeneratorASTDeclContextNameLookupTrait
Generator;
ASTDeclContextNameLookupTrait Trait(*this);
- // Store the sortable lookup results -- IE, those with meaningful
names. We
- // will sort them by the DeclarationName in order to stabilize the
ordering
- // of the hash table. We can't do this for constructors or conversion
- // functions which are handled separately.
- SmallVectorstd::pairDeclarationName, DeclContext::lookup_result,
16
- NamedResults;
-
- // We can't directly construct a nonce constructor or conversion
name without
- // tripping asserts, so we just recall the first one we see. Only
the kind
- // will actually be serialized.
+ // Create the on-disk hash table representation.
DeclarationName ConstructorName;
DeclarationName ConversionName;
- // Retain a mapping from each actual declaration name to the results
- // associated with it. We use a map here because the order in which
we
- // discover these lookup results isn't ordered. We have to
re-establish
- // a stable ordering which we do by walking the children of the decl
context,
- // and emitting these in the order in which their names first
appeared. Note
- // that the names' first appearance may not be one of the results or
a result
- // at all. We just use this to establish an ordering.
- llvm::SmallDenseMapDeclarationName, DeclContext::lookup_result, 8
- ConstructorResults;
- llvm::SmallDenseMapDeclarationName, DeclContext::lookup_result, 4
- ConversionResults;
+ SmallVectorNamedDecl *, 8 ConstructorDecls;
+ SmallVectorNamedDecl *, 4 ConversionDecls;
visitLocalLookupResults(DC, [](DeclarationName Name,
DeclContext::lookup_result Result) {
@@ -3805,90 +3786,34 @@ ASTWriter::GenerateNameLookupTable(const
// has the DeclarationName of the inherited constructors.
if (!ConstructorName)
ConstructorName = Name;
- ConstructorResults.insert(std::make_pair(Name, Result));
+ ConstructorDecls.append(Result.begin(), Result.end());
return;
case DeclarationName::CXXConversionFunctionName:
if (!ConversionName)
ConversionName = Name;
- ConversionResults.insert(std::make_pair(Name, Result));
+ ConversionDecls.append(Result.begin(), Result.end());
return;
default:
- NamedResults.push_back(std::make_pair(Name, Result));
+ break;
}
+Generator.insert(Name, Result, Trait);
});
- // Sort and emit the named results first. This is the easy case.
- std::sort(
- NamedResults.begin(), NamedResults.end(),
- [](const std::pairDeclarationName, DeclContext::lookup_result
LHS,
- const std::pairDeclarationName, DeclContext::lookup_result
RHS) {
-return LHS.first RHS.first;
- });
- for (auto Results :
Re: r233156 - [Modules] When writing out the on-disk hash table for the decl context
On Tue, Mar 24, 2015 at 7:07 PM, David Blaikie dblai...@gmail.com wrote: On Mar 24, 2015 5:42 PM, Chandler Carruth chandl...@gmail.com wrote: Author: chandlerc Date: Tue Mar 24 19:34:51 2015 New Revision: 233156 URL: http://llvm.org/viewvc/llvm-project?rev=233156view=rev Log: [Modules] When writing out the on-disk hash table for the decl context lookup tables, we need to establish a stable ordering for constructing the hash table. This is trickier than it might seem. Most of these cases are easily handled by sorting the lookup results associated with a specific name that has an identifier. However for constructors and conversion functions, the story is more complicated. Here we need to merge all of the constructors or conversion functions together and this merge needs to be stable. We don't have any stable ordering for either constructors or conversion functions as both would require a stable ordering across types. Instead, when we have constructors or conversion functions in the results, we reconstruct a stable order by walking the decl context in lexical order and merging them in the order their particular declaration names are encountered. I assume for user written ctors and conversion functions they appear in the order the user wrote them. What happens for implicitly defined ctors? Is their order guaranteed? This relies upon the order they occur in the AST. If that isn't deterministic, we'll also emit code for them and do other things in a non-deterministic way, so we can keep fixing up the stack. But I believe it is deterministic and based on the code which implicitly generates them. There is at least no hash table or other inherently non-deterministic data source there. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233249 - [Modules] A second attempt at writing out on-disk hash tables for the
Author: chandlerc
Date: Wed Mar 25 22:11:40 2015
New Revision: 233249
URL: http://llvm.org/viewvc/llvm-project?rev=233249view=rev
Log:
[Modules] A second attempt at writing out on-disk hash tables for the
decl context lookup tables.
The first attepmt at this caused problems. We had significantly more
sources of non-determinism that I realized at first, and my change
essentially turned them from non-deterministic output into
use-after-free. Except that they weren't necessarily caught by tools
because the data wasn't really freed.
The new approach is much simpler. The first big simplification is to
inline the visit code and handle this directly. That works much
better, and I'll try to go and clean up the other caller of the visit
logic similarly.
The second key to the entire approach is that we need to *only* collect
names into a stable order at first. We then need to issue all of the
actual 'lookup()' calls in the stable order of the names so that we load
external results in a stable order. Once we have loaded all the results,
the table of results will stop being invalidated and we can walk all of
the names again and use the cheap 'noload_lookup()' method to quickly
get the results and serialize them.
To handle constructors and conversion functions (whose names can't be
stably ordered) in this approach, what we do is record only the visible
constructor and conversion function names at first. Then, if we have
any, we walk the decls of the class and add those names in the order
they occur in the AST. The rest falls out naturally.
This actually ends up simpler than the previous approach and seems much
more robust.
It uncovered a latent issue where we were building on-disk hash tables
for lookup results when the context was a linkage spec! This happened to
dodge all of the assert by some miracle. Instead, add a proper predicate
to the DeclContext class and use that which tests both for function
contexts and linkage specs.
It also uncovered PR23030 where we are forming somewhat bizarre negative
lookup results. I've just worked around this with a FIXME in place
because fixing this particular Clang bug seems quite hard.
I've flipped the first part of the test case I added for stability back
on in this commit. I'm taking it gradually to try and make sure the
build bots are happy this time.
Modified:
cfe/trunk/include/clang/AST/DeclBase.h
cfe/trunk/include/clang/Serialization/ASTWriter.h
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/Modules/stress1.cpp
Modified: cfe/trunk/include/clang/AST/DeclBase.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=233249r1=233248r2=233249view=diff
==
--- cfe/trunk/include/clang/AST/DeclBase.h (original)
+++ cfe/trunk/include/clang/AST/DeclBase.h Wed Mar 25 22:11:40 2015
@@ -1209,6 +1209,11 @@ public:
}
}
+ /// \brief Test whether the context supports looking up names.
+ bool isLookupContext() const {
+return !isFunctionOrMethod() DeclKind != Decl::LinkageSpec;
+ }
+
bool isFileContext() const {
return DeclKind == Decl::TranslationUnit || DeclKind == Decl::Namespace;
}
Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTWriter.h?rev=233249r1=233248r2=233249view=diff
==
--- cfe/trunk/include/clang/Serialization/ASTWriter.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTWriter.h Wed Mar 25 22:11:40 2015
@@ -61,6 +61,7 @@ class PreprocessingRecord;
class Preprocessor;
class Sema;
class SourceManager;
+struct StoredDeclsList;
class SwitchCase;
class TargetInfo;
class Token;
@@ -505,6 +506,9 @@ private:
void WriteTypeAbbrevs();
void WriteType(QualType T);
+ bool isLookupResultExternal(StoredDeclsList Result, DeclContext *DC);
+ bool isLookupResultEntirelyExternal(StoredDeclsList Result, DeclContext
*DC);
+
templatetypename Visitor
void visitLocalLookupResults(const DeclContext *DC, Visitor AddLookupResult);
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233249r1=233248r2=233249view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Wed Mar 25 22:11:40 2015
@@ -3696,6 +3696,20 @@ public:
};
} // end anonymous namespace
+bool ASTWriter::isLookupResultExternal(StoredDeclsList Result,
+ DeclContext *DC) {
+ return Result.hasExternalDecls()
DC-NeedToReconcileExternalVisibleStorage;
+}
+
+bool ASTWriter::isLookupResultEntirelyExternal(StoredDeclsList Result,
+ DeclContext *DC) {
+ for (auto *D :
r233251 - [Modules] Delete a bunch of complex code for ensuring visible decls in
Author: chandlerc
Date: Wed Mar 25 23:27:10 2015
New Revision: 233251
URL: http://llvm.org/viewvc/llvm-project?rev=233251view=rev
Log:
[Modules] Delete a bunch of complex code for ensuring visible decls in
updated decl contexts get emitted.
Since this code was added, we have newer vastly simpler code for
handling this. The code I'm removing was very expensive and also
generated unstable order of declarations which made module outputs
non-deterministic.
All of the tests continue to pass for me and I'm able to check the
difference between the .pcm files after merging modules together.
Modified:
cfe/trunk/include/clang/Serialization/ASTWriter.h
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
cfe/trunk/test/Modules/stress1.cpp
Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTWriter.h?rev=233251r1=233250r2=233251view=diff
==
--- cfe/trunk/include/clang/Serialization/ASTWriter.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTWriter.h Wed Mar 25 23:27:10 2015
@@ -509,9 +509,6 @@ private:
bool isLookupResultExternal(StoredDeclsList Result, DeclContext *DC);
bool isLookupResultEntirelyExternal(StoredDeclsList Result, DeclContext
*DC);
- templatetypename Visitor
- void visitLocalLookupResults(const DeclContext *DC, Visitor AddLookupResult);
-
uint32_t GenerateNameLookupTable(const DeclContext *DC,
llvm::SmallVectorImplchar LookupTable);
uint64_t WriteDeclContextLexicalBlock(ASTContext Context, DeclContext *DC);
@@ -737,9 +734,6 @@ public:
/// \brief Add a version tuple to the given record
void AddVersionTuple(const VersionTuple Version, RecordDataImpl Record);
- /// \brief Mark a declaration context as needing an update.
- void AddUpdatedDeclContext(const DeclContext *DC);
-
void RewriteDecl(const Decl *D) {
DeclsToRewrite.insert(D);
}
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233251r1=233250r2=233251view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Wed Mar 25 23:27:10 2015
@@ -3710,57 +3710,6 @@ bool ASTWriter::isLookupResultEntirelyEx
return true;
}
-templatetypename Visitor
-void ASTWriter::visitLocalLookupResults(const DeclContext *ConstDC,
-Visitor AddLookupResult) {
- // FIXME: We need to build the lookups table, which is logically const.
- DeclContext *DC = const_castDeclContext*(ConstDC);
- assert(DC == DC-getPrimaryContext() only primary DC has lookup table);
-
- SmallVectorDeclarationName, 16 ExternalNames;
- for (auto Lookup : *DC-buildLookup()) {
-if (isLookupResultExternal(Lookup.second, DC)) {
- // If there are no local declarations in our lookup result, we don't
- // need to write an entry for the name at all unless we're rewriting
- // the decl context. If we can't write out a lookup set without
- // performing more deserialization, just skip this entry.
- if (!isRewritten(castDecl(DC))
- isLookupResultEntirelyExternal(Lookup.second, DC))
-continue;
-
- // We don't know for sure what declarations are found by this name,
- // because the external source might have a different set from the set
- // that are in the lookup map, and we can't update it now without
- // risking invalidating our lookup iterator. So add it to a queue to
- // deal with later.
- ExternalNames.push_back(Lookup.first);
- continue;
-}
-
-AddLookupResult(Lookup.first, Lookup.second.getLookupResult());
- }
-
- // Add the names we needed to defer. Note, this shouldn't add any new decls
- // to the list we need to serialize: any new declarations we find here should
- // be imported from an external source.
- // FIXME: What if the external source isn't an ASTReader?
- for (const auto Name : ExternalNames)
-AddLookupResult(Name, DC-lookup(Name));
-}
-
-void ASTWriter::AddUpdatedDeclContext(const DeclContext *DC) {
- if (UpdatedDeclContexts.insert(DC).second WritingAST) {
-// Ensure we emit all the visible declarations.
-// FIXME: This code is almost certainly wrong. It is at least failing to
-// visit all the decls it should.
-visitLocalLookupResults(DC, [](DeclarationName Name,
-DeclContext::lookup_result Result) {
- for (auto *Decl : Result)
-GetDeclRef(getDeclForLocalLookup(getLangOpts(), Decl));
-});
- }
-}
-
uint32_t
ASTWriter::GenerateNameLookupTable(const DeclContext *ConstDC,
llvm::SmallVectorImplchar LookupTable) {
@@
r233117 - [Modules] Stop creating timestamps for the modules cache and trying to
Author: chandlerc
Date: Tue Mar 24 16:44:25 2015
New Revision: 233117
URL: http://llvm.org/viewvc/llvm-project?rev=233117view=rev
Log:
[Modules] Stop creating timestamps for the modules cache and trying to
prune it when we have disabled implicit module generation and thus are
not using any cached modules.
Also update a test of explicitly generated modules to pass this CC1 flag
correctly.
This fixes an issue where Clang was dropping files into the source tree
while running its tests.
Modified:
cfe/trunk/lib/Frontend/CompilerInstance.cpp
cfe/trunk/test/Modules/relative-dep-gen.cpp
Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=233117r1=233116r2=233117view=diff
==
--- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Tue Mar 24 16:44:25 2015
@@ -1239,9 +1239,10 @@ void CompilerInstance::createModuleManag
if (!hasASTContext())
createASTContext();
-// If we're not recursively building a module, check whether we
-// need to prune the module cache.
-if (getSourceManager().getModuleBuildStack().empty()
+// If we're implicitly building modules but not currently recursively
+// building a module, check whether we need to prune the module cache.
+if (getLangOpts().ImplicitModules
+getSourceManager().getModuleBuildStack().empty()
getHeaderSearchOpts().ModuleCachePruneInterval 0
getHeaderSearchOpts().ModuleCachePruneAfter 0) {
pruneModuleCache(getHeaderSearchOpts());
Modified: cfe/trunk/test/Modules/relative-dep-gen.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/relative-dep-gen.cpp?rev=233117r1=233116r2=233117view=diff
==
--- cfe/trunk/test/Modules/relative-dep-gen.cpp (original)
+++ cfe/trunk/test/Modules/relative-dep-gen.cpp Tue Mar 24 16:44:25 2015
@@ -2,17 +2,17 @@
// RUN: rm -rf %t
// RUN: mkdir %t
//
-// RUN: %clang_cc1 -cc1 -fmodule-name=relative-dep-gen -emit-module -x c++
Inputs/relative-dep-gen.modulemap -dependency-file %t/build.d -MT mod.pcm -o
%t/mod.pcm
-// RUN: %clang_cc1 -cc1 -fmodule-map-file=Inputs/relative-dep-gen.modulemap
-fmodule-file=%t/mod.pcm -dependency-file %t/use-explicit.d -MT use.o
relative-dep-gen.cpp -fsyntax-only
-// RUN: %clang_cc1 -cc1 -fmodule-map-file=Inputs/relative-dep-gen.modulemap
-dependency-file %t/use-implicit.d relative-dep-gen.cpp -MT use.o -fsyntax-only
+// RUN: %clang_cc1 -cc1 -fno-implicit-modules -fmodule-name=relative-dep-gen
-emit-module -x c++ Inputs/relative-dep-gen.modulemap -dependency-file
%t/build.d -MT mod.pcm -o %t/mod.pcm
+// RUN: %clang_cc1 -cc1 -fno-implicit-modules
-fmodule-map-file=Inputs/relative-dep-gen.modulemap -fmodule-file=%t/mod.pcm
-dependency-file %t/use-explicit.d -MT use.o relative-dep-gen.cpp -fsyntax-only
+// RUN: %clang_cc1 -cc1 -fno-implicit-modules
-fmodule-map-file=Inputs/relative-dep-gen.modulemap -dependency-file
%t/use-implicit.d relative-dep-gen.cpp -MT use.o -fsyntax-only
//
// RUN: FileCheck --check-prefix=CHECK-BUILD %s %t/build.d
// RUN: FileCheck --check-prefix=CHECK-USE %s %t/use-explicit.d
// RUN: FileCheck --check-prefix=CHECK-USE %s %t/use-implicit.d
//
-// RUN: %clang_cc1 -cc1 -fmodule-name=relative-dep-gen -emit-module -x c++
Inputs/relative-dep-gen-cwd.modulemap -dependency-file %t/build-cwd.d -MT
mod.pcm -o %t/mod-cwd.pcm -fmodule-map-file-home-is-cwd
-// RUN: %clang_cc1 -cc1
-fmodule-map-file=Inputs/relative-dep-gen-cwd.modulemap
-fmodule-file=%t/mod-cwd.pcm -dependency-file %t/use-explicit-cwd.d -MT use.o
relative-dep-gen.cpp -fsyntax-only -fmodule-map-file-home-is-cwd
-// RUN: %clang_cc1 -cc1
-fmodule-map-file=Inputs/relative-dep-gen-cwd.modulemap -dependency-file
%t/use-implicit-cwd.d relative-dep-gen.cpp -MT use.o -fsyntax-only
-fmodule-map-file-home-is-cwd
+// RUN: %clang_cc1 -cc1 -fno-implicit-modules -fmodule-name=relative-dep-gen
-emit-module -x c++ Inputs/relative-dep-gen-cwd.modulemap -dependency-file
%t/build-cwd.d -MT mod.pcm -o %t/mod-cwd.pcm -fmodule-map-file-home-is-cwd
+// RUN: %clang_cc1 -cc1 -fno-implicit-modules
-fmodule-map-file=Inputs/relative-dep-gen-cwd.modulemap
-fmodule-file=%t/mod-cwd.pcm -dependency-file %t/use-explicit-cwd.d -MT use.o
relative-dep-gen.cpp -fsyntax-only -fmodule-map-file-home-is-cwd
+// RUN: %clang_cc1 -cc1 -fno-implicit-modules
-fmodule-map-file=Inputs/relative-dep-gen-cwd.modulemap -dependency-file
%t/use-implicit-cwd.d relative-dep-gen.cpp -MT use.o -fsyntax-only
-fmodule-map-file-home-is-cwd
//
// RUN: FileCheck --check-prefix=CHECK-BUILD %s %t/build-cwd.d
// RUN: FileCheck --check-prefix=CHECK-USE %s %t/use-explicit-cwd.d
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
r233163 - [Modules] Disable the diff of the merged module, there is still some
Author: chandlerc Date: Tue Mar 24 20:30:02 2015 New Revision: 233163 URL: http://llvm.org/viewvc/llvm-project?rev=233163view=rev Log: [Modules] Disable the diff of the merged module, there is still some non-determinism here, I just got lucky a bunch of times on my system. Modified: cfe/trunk/test/Modules/stress1.cpp Modified: cfe/trunk/test/Modules/stress1.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/stress1.cpp?rev=233163r1=233162r2=233163view=diff == --- cfe/trunk/test/Modules/stress1.cpp (original) +++ cfe/trunk/test/Modules/stress1.cpp Tue Mar 24 20:30:02 2015 @@ -60,7 +60,7 @@ // RUN: -emit-module -fmodule-name=merge00 -o %t/merge00_check.pcm \ // RUN: Inputs/stress1/module.modulemap // -// RUN: diff %t/merge00.pcm %t/merge00_check.pcm +// FIXME: diff %t/merge00.pcm %t/merge00_check.pcm // // RUN: %clang_cc1 -fmodules -x c++ -std=c++11 \ // RUN: -I Inputs/stress1 \ ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r233156 - [Modules] When writing out the on-disk hash table for the decl context
Author: chandlerc
Date: Tue Mar 24 19:34:51 2015
New Revision: 233156
URL: http://llvm.org/viewvc/llvm-project?rev=233156view=rev
Log:
[Modules] When writing out the on-disk hash table for the decl context
lookup tables, we need to establish a stable ordering for constructing
the hash table. This is trickier than it might seem.
Most of these cases are easily handled by sorting the lookup results
associated with a specific name that has an identifier. However for
constructors and conversion functions, the story is more complicated.
Here we need to merge all of the constructors or conversion functions
together and this merge needs to be stable. We don't have any stable
ordering for either constructors or conversion functions as both would
require a stable ordering across types.
Instead, when we have constructors or conversion functions in the
results, we reconstruct a stable order by walking the decl context in
lexical order and merging them in the order their particular declaration
names are encountered. This doesn't generalize as there might be found
declaration names which don't actually occur within the lexical context,
but for constructors and conversion functions it is safe. It does
require loading the entire decl context if necessary to establish the
ordering but there doesn't seem to be a meaningful way around that.
Many thanks to Richard for talking through all of the design choices
here. While I wrote the code, he guided all the actual decisions about
how to establish the order of things.
No test case yet because the test case I have doesn't pass yet -- there
are still more sources of non-determinism. However, this is complex
enough that I wanted it to go into its own commit in case it causes some
unforseen issue or needs to be reverted.
Modified:
cfe/trunk/lib/Serialization/ASTWriter.cpp
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233156r1=233155r2=233156view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Tue Mar 24 19:34:51 2015
@@ -3761,15 +3761,34 @@ ASTWriter::GenerateNameLookupTable(const
!DC-HasLazyExternalLexicalLookups
must call buildLookups first);
+ // Create the on-disk hash table representation.
llvm::OnDiskChainedHashTableGeneratorASTDeclContextNameLookupTrait
Generator;
ASTDeclContextNameLookupTrait Trait(*this);
- // Create the on-disk hash table representation.
+ // Store the sortable lookup results -- IE, those with meaningful names. We
+ // will sort them by the DeclarationName in order to stabilize the ordering
+ // of the hash table. We can't do this for constructors or conversion
+ // functions which are handled separately.
+ SmallVectorstd::pairDeclarationName, DeclContext::lookup_result, 16
+ NamedResults;
+
+ // We can't directly construct a nonce constructor or conversion name without
+ // tripping asserts, so we just recall the first one we see. Only the kind
+ // will actually be serialized.
DeclarationName ConstructorName;
DeclarationName ConversionName;
- SmallVectorNamedDecl *, 8 ConstructorDecls;
- SmallVectorNamedDecl *, 4 ConversionDecls;
+ // Retain a mapping from each actual declaration name to the results
+ // associated with it. We use a map here because the order in which we
+ // discover these lookup results isn't ordered. We have to re-establish
+ // a stable ordering which we do by walking the children of the decl context,
+ // and emitting these in the order in which their names first appeared. Note
+ // that the names' first appearance may not be one of the results or a result
+ // at all. We just use this to establish an ordering.
+ llvm::SmallDenseMapDeclarationName, DeclContext::lookup_result, 8
+ ConstructorResults;
+ llvm::SmallDenseMapDeclarationName, DeclContext::lookup_result, 4
+ ConversionResults;
visitLocalLookupResults(DC, [](DeclarationName Name,
DeclContext::lookup_result Result) {
@@ -3786,34 +3805,90 @@ ASTWriter::GenerateNameLookupTable(const
// has the DeclarationName of the inherited constructors.
if (!ConstructorName)
ConstructorName = Name;
- ConstructorDecls.append(Result.begin(), Result.end());
+ ConstructorResults.insert(std::make_pair(Name, Result));
return;
case DeclarationName::CXXConversionFunctionName:
if (!ConversionName)
ConversionName = Name;
- ConversionDecls.append(Result.begin(), Result.end());
+ ConversionResults.insert(std::make_pair(Name, Result));
return;
default:
- break;
+ NamedResults.push_back(std::make_pair(Name, Result));
}
-Generator.insert(Name, Result, Trait);
});
- // Add the constructors.
- if (!ConstructorDecls.empty()) {
+ //
r233162 - [Modules] Make the DeclUpdates map be processed in insertion order.
Author: chandlerc
Date: Tue Mar 24 20:02:12 2015
New Revision: 233162
URL: http://llvm.org/viewvc/llvm-project?rev=233162view=rev
Log:
[Modules] Make the DeclUpdates map be processed in insertion order.
This fixes my stress tests non-determinism so far. However, I've not
started playing with templates, friends, or terrible macros. I've found
at least two more seeming instabilities and am just waiting for a test
case to actually trigger them.
Added:
cfe/trunk/test/Modules/Inputs/stress1/
cfe/trunk/test/Modules/Inputs/stress1/common.h
cfe/trunk/test/Modules/Inputs/stress1/m00.h
cfe/trunk/test/Modules/Inputs/stress1/m01.h
cfe/trunk/test/Modules/Inputs/stress1/m02.h
cfe/trunk/test/Modules/Inputs/stress1/m03.h
cfe/trunk/test/Modules/Inputs/stress1/merge00.h
cfe/trunk/test/Modules/Inputs/stress1/module.modulemap
cfe/trunk/test/Modules/stress1.cpp
Modified:
cfe/trunk/include/clang/Serialization/ASTWriter.h
Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTWriter.h?rev=233162r1=233161r2=233162view=diff
==
--- cfe/trunk/include/clang/Serialization/ASTWriter.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTWriter.h Tue Mar 24 20:02:12 2015
@@ -321,7 +321,7 @@ private:
};
typedef SmallVectorDeclUpdate, 1 UpdateRecord;
- typedef llvm::DenseMapconst Decl *, UpdateRecord DeclUpdateMap;
+ typedef llvm::MapVectorconst Decl *, UpdateRecord DeclUpdateMap;
/// \brief Mapping from declarations that came from a chained PCH to the
/// record containing modifications to them.
DeclUpdateMap DeclUpdates;
Added: cfe/trunk/test/Modules/Inputs/stress1/common.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/stress1/common.h?rev=233162view=auto
==
--- cfe/trunk/test/Modules/Inputs/stress1/common.h (added)
+++ cfe/trunk/test/Modules/Inputs/stress1/common.h Tue Mar 24 20:02:12 2015
@@ -0,0 +1,38 @@
+#ifndef STRESS1_COMMON_H
+#define STRESS1_COMMON_H
+
+inline char function00(char x) { return x + x; }
+inline short function00(short x) { return x + x; }
+inline int function00(int x) { return x + x; }
+
+namespace N00 {
+struct S00 {
+ char c;
+ short s;
+ int i;
+
+ S00(char x) : c(x) {}
+ S00(short x) : s(x) {}
+ S00(int x) : i(x) {}
+
+ char method00(char x) { return x + x; }
+ short method00(short x) { return x + x; }
+ int method00(int x) { return x + x; }
+
+ operator char() { return c; }
+ operator short() { return s; }
+ operator int() { return i; }
+};
+struct S01 {};
+struct S03 {};
+}
+
+namespace N01 {
+struct S00 : N00::S00 {
+ using N00::S00::S00;
+};
+struct S01 {};
+struct S02 {};
+}
+
+#endif
Added: cfe/trunk/test/Modules/Inputs/stress1/m00.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/stress1/m00.h?rev=233162view=auto
==
--- cfe/trunk/test/Modules/Inputs/stress1/m00.h (added)
+++ cfe/trunk/test/Modules/Inputs/stress1/m00.h Tue Mar 24 20:02:12 2015
@@ -0,0 +1,6 @@
+#ifndef STRESS1_M00_H
+#define STRESS1_M00_H
+
+#include common.h
+
+#endif
Added: cfe/trunk/test/Modules/Inputs/stress1/m01.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/stress1/m01.h?rev=233162view=auto
==
--- cfe/trunk/test/Modules/Inputs/stress1/m01.h (added)
+++ cfe/trunk/test/Modules/Inputs/stress1/m01.h Tue Mar 24 20:02:12 2015
@@ -0,0 +1,6 @@
+#ifndef STRESS1_M01_H
+#define STRESS1_M01_H
+
+#include common.h
+
+#endif
Added: cfe/trunk/test/Modules/Inputs/stress1/m02.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/stress1/m02.h?rev=233162view=auto
==
--- cfe/trunk/test/Modules/Inputs/stress1/m02.h (added)
+++ cfe/trunk/test/Modules/Inputs/stress1/m02.h Tue Mar 24 20:02:12 2015
@@ -0,0 +1,6 @@
+#ifndef STRESS1_M02_H
+#define STRESS1_M02_H
+
+#include common.h
+
+#endif
Added: cfe/trunk/test/Modules/Inputs/stress1/m03.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/stress1/m03.h?rev=233162view=auto
==
--- cfe/trunk/test/Modules/Inputs/stress1/m03.h (added)
+++ cfe/trunk/test/Modules/Inputs/stress1/m03.h Tue Mar 24 20:02:12 2015
@@ -0,0 +1,6 @@
+#ifndef STRESS1_M03_H
+#define STRESS1_M03_H
+
+#include common.h
+
+#endif
Added: cfe/trunk/test/Modules/Inputs/stress1/merge00.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/stress1/merge00.h?rev=233162view=auto
==
---
r233115 - [Modules] Start making explicit modules produce deterministic output.
Author: chandlerc
Date: Tue Mar 24 16:18:10 2015
New Revision: 233115
URL: http://llvm.org/viewvc/llvm-project?rev=233115view=rev
Log:
[Modules] Start making explicit modules produce deterministic output.
There are two aspects of non-determinism fixed here, which was the
minimum required to cause at least an empty module to be deterministic.
First, the random number signature is only inserted into the module when
we are building modules implicitly. The use case for these random
signatures is to work around the very fact that modules are not
deterministic in their output when working with the implicitly built and
populated module cache. Eventually this should go away entirely when
we're confident that Clang is producing deterministic output.
Second, the on-disk hash table is populated based on the order of
iteration over a DenseMap. Instead, use a MapVector so that we can walk
it in insertion order.
I've added a test that an empty module, when built twice, produces the
same binary PCM file.
Added:
cfe/trunk/test/Modules/Inputs/empty/
cfe/trunk/test/Modules/Inputs/empty/empty.h
cfe/trunk/test/Modules/empty.modulemap
Modified:
cfe/trunk/include/clang/Serialization/ASTWriter.h
cfe/trunk/lib/Serialization/ASTWriter.cpp
Modified: cfe/trunk/include/clang/Serialization/ASTWriter.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTWriter.h?rev=233115r1=233114r2=233115view=diff
==
--- cfe/trunk/include/clang/Serialization/ASTWriter.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTWriter.h Tue Mar 24 16:18:10 2015
@@ -225,7 +225,7 @@ private:
/// The ID numbers for identifiers are consecutive (in order of
/// discovery), starting at 1. An ID of zero refers to a NULL
/// IdentifierInfo.
- llvm::DenseMapconst IdentifierInfo *, serialization::IdentID IdentifierIDs;
+ llvm::MapVectorconst IdentifierInfo *, serialization::IdentID
IdentifierIDs;
/// \brief The first ID number we can use for our own macros.
serialization::MacroID FirstMacroID;
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233115r1=233114r2=233115view=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Tue Mar 24 16:18:10 2015
@@ -1160,12 +1160,17 @@ void ASTWriter::WriteControlBlock(Prepro
Stream.EmitRecordWithBlob(MetadataAbbrevCode, Record,
getClangFullRepositoryVersion());
- // Signature
- Record.clear();
- Record.push_back(getSignature());
- Stream.EmitRecord(SIGNATURE, Record);
-
if (WritingModule) {
+// For implicit modules we output a signature that we can use to ensure
+// duplicate module builds don't collide in the cache as their output order
+// is non-deterministic.
+// FIXME: Remove this when output is deterministic.
+if (Context.getLangOpts().ImplicitModules) {
+ Record.clear();
+ Record.push_back(getSignature());
+ Stream.EmitRecord(SIGNATURE, Record);
+}
+
// Module name
BitCodeAbbrev *Abbrev = new BitCodeAbbrev();
Abbrev-Add(BitCodeAbbrevOp(MODULE_NAME));
@@ -3507,14 +3512,12 @@ void ASTWriter::WriteIdentifierTable(Pre
// Create the on-disk hash table representation. We only store offsets
// for identifiers that appear here for the first time.
IdentifierOffsets.resize(NextIdentID - FirstIdentID);
-for (llvm::DenseMapconst IdentifierInfo *, IdentID::iterator
- ID = IdentifierIDs.begin(), IDEnd = IdentifierIDs.end();
- ID != IDEnd; ++ID) {
- assert(ID-first NULL identifier in identifier table);
- if (!Chain || !ID-first-isFromAST() ||
- ID-first-hasChangedSinceDeserialization())
-Generator.insert(const_castIdentifierInfo *(ID-first), ID-second,
- Trait);
+for (auto IdentIDPair : IdentifierIDs) {
+ IdentifierInfo *II = const_castIdentifierInfo *(IdentIDPair.first);
+ IdentID ID = IdentIDPair.second;
+ assert(II NULL identifier in identifier table);
+ if (!Chain || !II-isFromAST() || II-hasChangedSinceDeserialization())
+Generator.insert(II, ID, Trait);
}
// Create the on-disk hash table in a buffer.
Added: cfe/trunk/test/Modules/Inputs/empty/empty.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/empty/empty.h?rev=233115view=auto
==
--- cfe/trunk/test/Modules/Inputs/empty/empty.h (added)
+++ cfe/trunk/test/Modules/Inputs/empty/empty.h Tue Mar 24 16:18:10 2015
@@ -0,0 +1 @@
+// This file intentionally left empty.
Added: cfe/trunk/test/Modules/empty.modulemap
URL:
Re: r220493 - Add a signature to AST files to verify that they haven't changed
On Mon, Mar 23, 2015 at 10:37 PM, Ben Langmuir blangm...@apple.com wrote: On Mar 23, 2015, at 10:21 PM, Chandler Carruth chandl...@google.com wrote: On Mon, Mar 23, 2015 at 9:54 PM, Ben Langmuir blangm...@apple.com wrote: On Mar 23, 2015, at 5:38 PM, Chandler Carruth chandl...@google.com wrote: Digging this thread back up, I'm starting to work on deterministic output of modules. This is *really* important. Unfortunately, it's almost impossible to make progress as long as this code is actually firing because it kind of makes things definitively non-deterministic. ;] Ben, do you have any concerns about only emitting the signature record for implicit modules? This would omit it for explicit modules (where I'm working), PCH files, etc. Based on your commit message and explanation, it doesn't really make sense outside of implicit modules AFAICT, and we're hoping it will go away completely. SGTM although I would prefer we continue to check the /expected/ signature when an AST file imports an implicit module (and in particular when a PCH imports an implicit module). Thanks for working on this! I'm not planning to change the checking code path at all. All the tests pass if I just restrict the emission to be when generating implicit modules Perfect, that’s what I was hoping for. Sorry I didn’t phrase it clearly. This is in as part of r233115 with the minimal test case for deterministic output. More to come. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r220493 - Add a signature to AST files to verify that they haven't changed
On Mon, Mar 23, 2015 at 9:54 PM, Ben Langmuir blangm...@apple.com wrote:
On Mar 23, 2015, at 5:38 PM, Chandler Carruth chandl...@google.com
wrote:
Digging this thread back up, I'm starting to work on deterministic output
of modules. This is *really* important. Unfortunately, it's almost
impossible to make progress as long as this code is actually firing because
it kind of makes things definitively non-deterministic. ;]
Ben, do you have any concerns about only emitting the signature record for
implicit modules? This would omit it for explicit modules (where I'm
working), PCH files, etc. Based on your commit message and explanation, it
doesn't really make sense outside of implicit modules AFAICT, and we're
hoping it will go away completely.
SGTM although I would prefer we continue to check the /expected/ signature
when an AST file imports an implicit module (and in particular when a PCH
imports an implicit module). Thanks for working on this!
I'm not planning to change the checking code path at all. All the tests
pass if I just restrict the emission to be when generating implicit modules.
Perhaps the confusion is that we have two totally different
'implicit/explicit' things in Modules land. There are implicit vs. explicit
submodules. That's not what I'm talking about. I'm talking about whether
-fno-implicit-modules is passed. When the build system is produces modules
files for us, it doesn't seem like we ever need to take responsibility for
the signature and can just omit it.
At least, all the regression tests pass when I make the above change, so
even if it isn't the perfect state, I think it is a strict improvement. It
at least allows me to test the removal of some other bit of non-determinism.
Also, I have one question here which may just be my ignorance:
On Thu, Oct 23, 2014 at 11:05 AM, Ben Langmuir blangm...@apple.com
wrote:
+ASTFileSignature ExpectedSignature,
+
std::functionASTFileSignature(llvm::BitstreamReader )
+ReadSignature,
Wow, a std::function? Is this really necessary? Seems super heavy weight.
Oops, totally unnecessary. Strength-reduced to a function pointer
in r233050.
+static ASTFileSignature readASTFileSignature(llvm::BitstreamReader
StreamFile){
+ BitstreamCursor Stream(StreamFile);
+ if (Stream.Read(8) != 'C' ||
+ Stream.Read(8) != 'P' ||
+ Stream.Read(8) != 'C' ||
+ Stream.Read(8) != 'H') {
+return 0;
+ }
This is truly magical. I assume this is checking for some magic string?
Comments or something would be really nice here. Sharing the code would be
even more nice.
Yeah, this is the AST/PCH file magic number. Added
“startsWithASTFileMagic” to factor this out of the three places we check
this, also in r233050.
+
+ // Scan for the CONTROL_BLOCK_ID block.
+ if (SkipCursorToBlock(Stream, CONTROL_BLOCK_ID))
+return 0;
Why would it ever be reasonable to call this for a module file without
such a block?
It’s not, but that’s exactly what returning 0 means - I added a doc
comment to that effect. Or was there something else here?
If this is a programming invariant, it should be assert()-ed rather than
returning something.
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r220493 - Add a signature to AST files to verify that they haven't changed
On Mon, Mar 23, 2015 at 10:37 PM, Ben Langmuir blangm...@apple.com wrote:
On Mar 23, 2015, at 10:21 PM, Chandler Carruth chandl...@google.com
wrote:
On Mon, Mar 23, 2015 at 9:54 PM, Ben Langmuir blangm...@apple.com wrote:
On Mar 23, 2015, at 5:38 PM, Chandler Carruth chandl...@google.com
wrote:
Digging this thread back up, I'm starting to work on deterministic output
of modules. This is *really* important. Unfortunately, it's almost
impossible to make progress as long as this code is actually firing because
it kind of makes things definitively non-deterministic. ;]
Ben, do you have any concerns about only emitting the signature record
for implicit modules? This would omit it for explicit modules (where I'm
working), PCH files, etc. Based on your commit message and explanation, it
doesn't really make sense outside of implicit modules AFAICT, and we're
hoping it will go away completely.
SGTM although I would prefer we continue to check the /expected/
signature when an AST file imports an implicit module (and in particular
when a PCH imports an implicit module). Thanks for working on this!
I'm not planning to change the checking code path at all. All the tests
pass if I just restrict the emission to be when generating implicit modules
Perfect, that’s what I was hoping for. Sorry I didn’t phrase it clearly.
Perhaps the confusion is that we have two totally different
'implicit/explicit' things in Modules land. There are implicit vs. explicit
submodules. That's not what I'm talking about. I'm talking about whether
-fno-implicit-modules is passed. When the build system is produces modules
files for us, it doesn't seem like we ever need to take responsibility for
the signature and can just omit it.
At least, all the regression tests pass when I make the above change, so
even if it isn't the perfect state, I think it is a strict improvement. It
at least allows me to test the removal of some other bit of non-determinism.
Also, I have one question here which may just be my ignorance:
On Thu, Oct 23, 2014 at 11:05 AM, Ben Langmuir blangm...@apple.com
wrote:
+ASTFileSignature ExpectedSignature,
+
std::functionASTFileSignature(llvm::BitstreamReader )
+ReadSignature,
Wow, a std::function? Is this really necessary? Seems super heavy weight.
Oops, totally unnecessary. Strength-reduced to a function pointer
in r233050.
+static ASTFileSignature readASTFileSignature(llvm::BitstreamReader
StreamFile){
+ BitstreamCursor Stream(StreamFile);
+ if (Stream.Read(8) != 'C' ||
+ Stream.Read(8) != 'P' ||
+ Stream.Read(8) != 'C' ||
+ Stream.Read(8) != 'H') {
+return 0;
+ }
This is truly magical. I assume this is checking for some magic string?
Comments or something would be really nice here. Sharing the code would be
even more nice.
Yeah, this is the AST/PCH file magic number. Added
“startsWithASTFileMagic” to factor this out of the three places we check
this, also in r233050.
+
+ // Scan for the CONTROL_BLOCK_ID block.
+ if (SkipCursorToBlock(Stream, CONTROL_BLOCK_ID))
+return 0;
Why would it ever be reasonable to call this for a module file without
such a block?
It’s not, but that’s exactly what returning 0 means - I added a doc
comment to that effect. Or was there something else here?
If this is a programming invariant, it should be assert()-ed rather than
returning something.
It’s an error condition not an invariant, since we’re reading a file
provided by the user. The caller of this function produces a fatal error
if it returns 0. Admittedly we aren’t very paranoid about malformed AST
files in general, but an error seems better when we can trivially provide
one.
If we want to diagnose malformed AST files, we should be using a
diagnostic, not a return of zero.
The AST files are essentially programmatic interfaces between Clang and
itself. I'm not aware of any realistic handling of malformed inputs like
this. If we want to do this, we should do it correctly. If we're not
actually doing it, it seems quite misleading to return a bizarre error
number.
What prevents a random number of '0' from actually *being* the signature?
None of this seems really principled in its design, and as something that
is in trunk for many months now, I think its principles need to be shored
up.
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: r220493 - Add a signature to AST files to verify that they haven't changed
Digging this thread back up, I'm starting to work on deterministic output
of modules. This is *really* important. Unfortunately, it's almost
impossible to make progress as long as this code is actually firing because
it kind of makes things definitively non-deterministic. ;]
Ben, do you have any concerns about only emitting the signature record for
implicit modules? This would omit it for explicit modules (where I'm
working), PCH files, etc. Based on your commit message and explanation, it
doesn't really make sense outside of implicit modules AFAICT, and we're
hoping it will go away completely.
Also, I have one question here which may just be my ignorance:
On Thu, Oct 23, 2014 at 11:05 AM, Ben Langmuir blangm...@apple.com wrote:
+ASTFileSignature ExpectedSignature,
+
std::functionASTFileSignature(llvm::BitstreamReader )
+ReadSignature,
Wow, a std::function? Is this really necessary? Seems super heavy weight.
+static ASTFileSignature readASTFileSignature(llvm::BitstreamReader
StreamFile){
+ BitstreamCursor Stream(StreamFile);
+ if (Stream.Read(8) != 'C' ||
+ Stream.Read(8) != 'P' ||
+ Stream.Read(8) != 'C' ||
+ Stream.Read(8) != 'H') {
+return 0;
+ }
This is truly magical. I assume this is checking for some magic string?
Comments or something would be really nice here. Sharing the code would be
even more nice.
+
+ // Scan for the CONTROL_BLOCK_ID block.
+ if (SkipCursorToBlock(Stream, CONTROL_BLOCK_ID))
+return 0;
Why would it ever be reasonable to call this for a module file without such
a block?
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r232778 - [Modules] Implement __builtin_isinf_sign in Clang.
Author: chandlerc
Date: Thu Mar 19 17:39:51 2015
New Revision: 232778
URL: http://llvm.org/viewvc/llvm-project?rev=232778view=rev
Log:
[Modules] Implement __builtin_isinf_sign in Clang.
Somehow, we never managed to implement this fully. We could constant
fold it like crazy, including constant folding complex arguments, etc.
But if you actually needed to generate code for it, error.
I've implemented it using the somewhat obvious lowering. Happy for
suggestions on a more clever way to lower this.
Now, what you might ask does this have to do with modules? Fun story. So
it turns out that libstdc++ actually uses __builtin_isinf_sign to
implement std::isinf when in C++98 mode, but only inside of a template.
So if we're lucky, and we never instantiate that, everything is good.
But once we try to instantiate that template function, we need this
builtin. All of my customers at least are using C++11 and so they never
hit this code path.
But what does that have to do with modules? Fun story. So it turns out
that with modules we actually observe a bunch of bugs in libstdc++ where
their cmath header clobbers things exposed by math.h. To fix these,
we have to provide global function definitions to replace the macros
that C99 would have used. And it turns out that ::isinf needs to be
implemented using the exact semantics used by the C++98 variant of
std::isinf. And so I started to fix this bug in libstdc++ and ceased to
be able to compile libstdc++ with Clang.
The yaks are legion.
Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/test/CodeGen/builtins.c
Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=232778r1=232777r2=232778view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Thu Mar 19 17:39:51 2015
@@ -158,6 +158,27 @@ static Value *EmitFAbs(CodeGenFunction
return Call;
}
+/// Emit the computation of the sign bit for a floating point value. Returns
+/// the i1 sign bit value.
+static Value *EmitSignBit(CodeGenFunction CGF, Value *V) {
+ LLVMContext C = CGF.CGM.getLLVMContext();
+
+ llvm::Type *Ty = V-getType();
+ int Width = Ty-getPrimitiveSizeInBits();
+ llvm::Type *IntTy = llvm::IntegerType::get(C, Width);
+ V = CGF.Builder.CreateBitCast(V, IntTy);
+ if (Ty-isPPC_FP128Ty()) {
+// The higher-order double comes first, and so we need to truncate the
+// pair to extract the overall sign. The order of the pair is the same
+// in both little- and big-Endian modes.
+Width = 1;
+IntTy = llvm::IntegerType::get(C, Width);
+V = CGF.Builder.CreateTrunc(V, IntTy);
+ }
+ Value *Zero = llvm::Constant::getNullValue(IntTy);
+ return CGF.Builder.CreateICmpSLT(V, Zero);
+}
+
static RValue emitLibraryCall(CodeGenFunction CGF, const FunctionDecl *Fn,
const CallExpr *E, llvm::Value *calleeValue) {
return CGF.EmitCall(E-getCallee()-getType(), calleeValue, E,
@@ -558,8 +579,22 @@ RValue CodeGenFunction::EmitBuiltinExpr(
return RValue::get(Builder.CreateZExt(V, ConvertType(E-getType(;
}
- // TODO: BI__builtin_isinf_sign
- // isinf_sign(x) - isinf(x) ? (signbit(x) ? -1 : 1) : 0
+ case Builtin::BI__builtin_isinf_sign: {
+// isinf_sign(x) - fabs(x) == infinity ? (signbit(x) ? -1 : 1) : 0
+Value *Arg = EmitScalarExpr(E-getArg(0));
+Value *AbsArg = EmitFAbs(*this, Arg);
+Value *IsInf = Builder.CreateFCmpOEQ(
+AbsArg, ConstantFP::getInfinity(Arg-getType()), isinf);
+Value *IsNeg = EmitSignBit(*this, Arg);
+
+llvm::Type *IntTy = ConvertType(E-getType());
+Value *Zero = Constant::getNullValue(IntTy);
+Value *One = ConstantInt::get(IntTy, 1);
+Value *NegativeOne = ConstantInt::get(IntTy, -1);
+Value *SignResult = Builder.CreateSelect(IsNeg, NegativeOne, One);
+Value *Result = Builder.CreateSelect(IsInf, SignResult, Zero);
+return RValue::get(Result);
+ }
case Builtin::BI__builtin_isnormal: {
// isnormal(x) -- x == x fabsf(x) infinity fabsf(x) = float_min
@@ -1398,24 +1433,9 @@ RValue CodeGenFunction::EmitBuiltinExpr(
case Builtin::BI__builtin_signbit:
case Builtin::BI__builtin_signbitf:
case Builtin::BI__builtin_signbitl: {
-LLVMContext C = CGM.getLLVMContext();
-
-Value *Arg = EmitScalarExpr(E-getArg(0));
-llvm::Type *ArgTy = Arg-getType();
-int ArgWidth = ArgTy-getPrimitiveSizeInBits();
-llvm::Type *ArgIntTy = llvm::IntegerType::get(C, ArgWidth);
-Value *BCArg = Builder.CreateBitCast(Arg, ArgIntTy);
-if (ArgTy-isPPC_FP128Ty()) {
- // The higher-order double comes first, and so we need to truncate the
- // pair to extract the overall sign. The order of the pair is the same
- // in both little- and big-Endian modes.
- ArgWidth = 1;
- ArgIntTy = llvm::IntegerType::get(C, ArgWidth);
- BCArg =
