[PATCH] D55250: [clangd] Enhance macro hover to see full definition
This revision was automatically updated to reflect the committed changes.
Closed by commit rL354761: [clangd] Enhance macro hover to see full definition
(authored by malaperle, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D55250?vs=187902=188095#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55250/new/
https://reviews.llvm.org/D55250
Files:
clang-tools-extra/trunk/clangd/XRefs.cpp
clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
Index: clang-tools-extra/trunk/clangd/XRefs.cpp
===
--- clang-tools-extra/trunk/clangd/XRefs.cpp
+++ clang-tools-extra/trunk/clangd/XRefs.cpp
@@ -558,13 +558,30 @@
return H;
}
-/// Generate a \p Hover object given the macro \p MacroInf.
-static Hover getHoverContents(llvm::StringRef MacroName) {
- Hover H;
-
- H.contents.value = "#define ";
- H.contents.value += MacroName;
+/// Generate a \p Hover object given the macro \p MacroDecl.
+static Hover getHoverContents(MacroDecl Decl, ParsedAST ) {
+ SourceManager = AST.getASTContext().getSourceManager();
+ std::string Definition = Decl.Name;
+
+ // Try to get the full definition, not just the name
+ SourceLocation StartLoc = Decl.Info->getDefinitionLoc();
+ SourceLocation EndLoc = Decl.Info->getDefinitionEndLoc();
+ if (EndLoc.isValid()) {
+EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM,
+AST.getASTContext().getLangOpts());
+bool Invalid;
+StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), );
+if (!Invalid) {
+ unsigned StartOffset = SM.getFileOffset(StartLoc);
+ unsigned EndOffset = SM.getFileOffset(EndLoc);
+ if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
+Definition = Buffer.substr(StartOffset, EndOffset - StartOffset).str();
+}
+ }
+ Hover H;
+ H.contents.kind = MarkupKind::PlainText;
+ H.contents.value = "#define " + Definition;
return H;
}
@@ -688,7 +705,7 @@
auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
if (!Symbols.Macros.empty())
-return getHoverContents(Symbols.Macros[0].Name);
+return getHoverContents(Symbols.Macros[0], AST);
if (!Symbols.Decls.empty())
return getHoverContents(Symbols.Decls[0]);
Index: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
@@ -743,7 +743,25 @@
#define MACRO 2
#undef macro
)cpp",
- "#define MACRO",
+ "#define MACRO 1",
+ },
+ {
+ R"cpp(// Macro
+#define MACRO 0
+#define MACRO2 ^MACRO
+ )cpp",
+ "#define MACRO 0",
+ },
+ {
+ R"cpp(// Macro
+#define MACRO {\
+ return 0;\
+}
+int main() ^MACRO
+ )cpp",
+ R"cpp(#define MACRO {\
+ return 0;\
+})cpp",
},
{
R"cpp(// Forward class declaration
Index: clang-tools-extra/trunk/clangd/XRefs.cpp
===
--- clang-tools-extra/trunk/clangd/XRefs.cpp
+++ clang-tools-extra/trunk/clangd/XRefs.cpp
@@ -558,13 +558,30 @@
return H;
}
-/// Generate a \p Hover object given the macro \p MacroInf.
-static Hover getHoverContents(llvm::StringRef MacroName) {
- Hover H;
-
- H.contents.value = "#define ";
- H.contents.value += MacroName;
+/// Generate a \p Hover object given the macro \p MacroDecl.
+static Hover getHoverContents(MacroDecl Decl, ParsedAST ) {
+ SourceManager = AST.getASTContext().getSourceManager();
+ std::string Definition = Decl.Name;
+
+ // Try to get the full definition, not just the name
+ SourceLocation StartLoc = Decl.Info->getDefinitionLoc();
+ SourceLocation EndLoc = Decl.Info->getDefinitionEndLoc();
+ if (EndLoc.isValid()) {
+EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM,
+AST.getASTContext().getLangOpts());
+bool Invalid;
+StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), );
+if (!Invalid) {
+ unsigned StartOffset = SM.getFileOffset(StartLoc);
+ unsigned EndOffset = SM.getFileOffset(EndLoc);
+ if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
+Definition = Buffer.substr(StartOffset, EndOffset - StartOffset).str();
+}
+ }
+ Hover H;
+ H.contents.kind = MarkupKind::PlainText;
+ H.contents.value = "#define " + Definition;
return H;
}
@@ -688,7 +705,7 @@
auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
if (!Symbols.Macros.empty())
-return getHoverContents(Symbols.Macros[0].Name);
+return
[clang-tools-extra] r354761 - [clangd] Enhance macro hover to see full definition
Author: malaperle
Date: Sun Feb 24 15:47:03 2019
New Revision: 354761
URL: http://llvm.org/viewvc/llvm-project?rev=354761=rev
Log:
[clangd] Enhance macro hover to see full definition
Summary: Signed-off-by: Marc-Andre Laperle
Reviewers: simark, ilya-biryukov, sammccall, ioeric, hokein
Reviewed By: ilya-biryukov
Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, kadircet,
cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D55250
Modified:
clang-tools-extra/trunk/clangd/XRefs.cpp
clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
Modified: clang-tools-extra/trunk/clangd/XRefs.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/XRefs.cpp?rev=354761=354760=354761=diff
==
--- clang-tools-extra/trunk/clangd/XRefs.cpp (original)
+++ clang-tools-extra/trunk/clangd/XRefs.cpp Sun Feb 24 15:47:03 2019
@@ -558,13 +558,30 @@ static Hover getHoverContents(QualType T
return H;
}
-/// Generate a \p Hover object given the macro \p MacroInf.
-static Hover getHoverContents(llvm::StringRef MacroName) {
- Hover H;
-
- H.contents.value = "#define ";
- H.contents.value += MacroName;
+/// Generate a \p Hover object given the macro \p MacroDecl.
+static Hover getHoverContents(MacroDecl Decl, ParsedAST ) {
+ SourceManager = AST.getASTContext().getSourceManager();
+ std::string Definition = Decl.Name;
+
+ // Try to get the full definition, not just the name
+ SourceLocation StartLoc = Decl.Info->getDefinitionLoc();
+ SourceLocation EndLoc = Decl.Info->getDefinitionEndLoc();
+ if (EndLoc.isValid()) {
+EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM,
+AST.getASTContext().getLangOpts());
+bool Invalid;
+StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), );
+if (!Invalid) {
+ unsigned StartOffset = SM.getFileOffset(StartLoc);
+ unsigned EndOffset = SM.getFileOffset(EndLoc);
+ if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
+Definition = Buffer.substr(StartOffset, EndOffset - StartOffset).str();
+}
+ }
+ Hover H;
+ H.contents.kind = MarkupKind::PlainText;
+ H.contents.value = "#define " + Definition;
return H;
}
@@ -688,7 +705,7 @@ llvm::Optional getHover(ParsedAST
auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
if (!Symbols.Macros.empty())
-return getHoverContents(Symbols.Macros[0].Name);
+return getHoverContents(Symbols.Macros[0], AST);
if (!Symbols.Decls.empty())
return getHoverContents(Symbols.Decls[0]);
Modified: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp?rev=354761=354760=354761=diff
==
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp Sun Feb 24 15:47:03
2019
@@ -743,7 +743,25 @@ TEST(Hover, All) {
#define MACRO 2
#undef macro
)cpp",
- "#define MACRO",
+ "#define MACRO 1",
+ },
+ {
+ R"cpp(// Macro
+#define MACRO 0
+#define MACRO2 ^MACRO
+ )cpp",
+ "#define MACRO 0",
+ },
+ {
+ R"cpp(// Macro
+#define MACRO {\
+ return 0;\
+}
+int main() ^MACRO
+ )cpp",
+ R"cpp(#define MACRO {\
+ return 0;\
+})cpp",
},
{
R"cpp(// Forward class declaration
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls
a_sidorin added a comment.
Hi Gabor,
The patch looks OK overall but I have some comments inline.
Comment at: lib/AST/ASTImporter.cpp:4966
// it has any definition in the redecl chain.
-static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) {
- CXXRecordDecl *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
+template static auto getDefinition(T *D) -> T * {
+ auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
We should point that this function is for TemplateDecls only somehow. But we
can't just pass TemplateDecl as the parameter due to loss of the actual return
type. Maybewe should rename this function into "getTemplateDefinition()"?
Comment at: lib/AST/ASTImporter.cpp:5563
+ // TODO: handle conflicting names
+} // linkage
+ } // template
We don't usually put such comments after control flow statements. If they are
really needed, it is a good sign that a function must be split, and it's better
to leave a FIXME for this (or do the split).
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58494/new/
https://reviews.llvm.org/D58494
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58502: [ASTImporter] Fix redecl failures of Class and ClassTemplate
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, I don't see any problems with the patch. Thanks! I think it will be good to get Shafik's approval as well. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58502/new/ https://reviews.llvm.org/D58502 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path
krytarowski accepted this revision. krytarowski added a comment. This revision is now accepted and ready to land. This will make life much more easier now with this change. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58592/new/ https://reviews.llvm.org/D58592 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r354752 - Fix accidentally used hard tabs. NFC
Author: kristina Date: Sun Feb 24 10:06:10 2019 New Revision: 354752 URL: http://llvm.org/viewvc/llvm-project?rev=354752=rev Log: Fix accidentally used hard tabs. NFC Big sorry. This undoes the indentation mess I made in r354751. Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=354752=354751=354752=diff == --- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original) +++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Sun Feb 24 10:06:10 2019 @@ -2007,7 +2007,7 @@ RValue CodeGenFunction::EmitBuiltinExpr( unsigned Alignment = (unsigned)AlignmentCI->getZExtValue(); EmitAlignmentAssumption(PtrValue, Ptr, - /*The expr loc is sufficient.*/ SourceLocation(), +/*The expr loc is sufficient.*/ SourceLocation(), Alignment, OffsetValue); return RValue::get(PtrValue); } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods
Hey David Thanks for letting me know, and analysing it this far! I also can't see anything wrong with the intrinsic. Its just defined as: def int_objc_autoreleasePoolPop : Intrinsic<[], [llvm_ptr_ty]>; which (I believe) means it has unmodelled side effects so it should have been fine for your example. I'll try build the same file you did and see if I can reproduce. Cheers, Pete > On Feb 24, 2019, at 7:48 AM, David Chisnall via Phabricator > wrote: > > theraven added a comment. > Herald added a project: LLVM. > > After some bisection, it appears that this is the revision that introduced > the regression in the GNUstep Objective-C runtime test suite that I reported > on the list a few weeks ago. In this is the test (compiled with > `-fobjc-runtime=gnustep-2.0 -O3` and an ELF triple): > > https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m > > After this change, Early CSE w/ MemorySSA is determining that the second load > of `deallocCalled` is redundant. The code goes from: > >%7 = load i1, i1* @deallocCalled, align 1 >br i1 %7, label %8, label %9 > > ; :8: ; preds = %0 >call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* > @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x > i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8], [15 > x i8]* @.str.1, i64 0, i64 0)) #5 >unreachable > > ; :9: ; preds = %0 >call void @llvm.objc.autoreleasePoolPop(i8* %1) >%10 = load i1, i1* @deallocCalled, align 1 >br i1 %10, label %12, label %11 > > ; :11: ; preds = %9 >call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* > @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x > i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8], [14 > x i8]* @.str.2, i64 0, i64 0)) #5 >unreachable > > to: > >%7 = load i1, i1* @deallocCalled, align 1 >br i1 %7, label %8, label %9 > > ; :8: ; preds = %0 >call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* > @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x > i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8], [15 > x i8]* @.str.1, i64 0, i64 0)) #5 >unreachable > > ; :9: ; preds = %0 >call void @llvm.objc.autoreleasePoolPop(i8* %1) >br i1 %7, label %11, label %10 > > ; :10: ; preds = %9 >call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* > @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x > i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8], [14 > x i8]* @.str.2, i64 0, i64 0)) #5 >unreachable > > Later optimisations then determine that, because the assert does not return, > the only possible value for %7 is false and cause the second assert to fire > unconditionally. > > It appears that we are not correctly modelling the side effects of the > `llvm.objc.autoreleasePoolPop` intrinsic, but it's not entirely clear why > not. The same test compiled for the macos runtime does not appear to exhibit > the same behaviour. The previous revision, where we emitted a call to > `objc_autoreleasePoolPop` and not the intrinsic worked correctly, but with > this change the optimisers are assuming that no globals can be modified > across an autorelease pool pop operation (at least, in some situations). > > Looking at the definition of the intrinsic, I don't see anything wrong, so I > still suspect that there is a MemorySSA bug that this has uncovered, rather > than anything wrong in this series of commits. Any suggestions as to where > to look would be appreciated. > > > Repository: > rL LLVM > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D55802/new/ > > https://reviews.llvm.org/D55802 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r354751 - Wrap code for builtin_assume_aligned at 80 col.NFC
Author: kristina Date: Sun Feb 24 09:57:33 2019 New Revision: 354751 URL: http://llvm.org/viewvc/llvm-project?rev=354751=rev Log: Wrap code for builtin_assume_aligned at 80 col.NFC Minor style fix to avoid going over 80 cols in handling of case for Builtin::BI__builtin_assume_aligned. NFC. Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=354751=354750=354751=diff == --- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original) +++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Sun Feb 24 09:57:33 2019 @@ -2006,7 +2006,8 @@ RValue CodeGenFunction::EmitBuiltinExpr( ConstantInt *AlignmentCI = cast(AlignmentValue); unsigned Alignment = (unsigned)AlignmentCI->getZExtValue(); -EmitAlignmentAssumption(PtrValue, Ptr, /*The expr loc is sufficient.*/ SourceLocation(), +EmitAlignmentAssumption(PtrValue, Ptr, + /*The expr loc is sufficient.*/ SourceLocation(), Alignment, OffsetValue); return RValue::get(PtrValue); } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path
mgorny updated this revision to Diff 188083.
mgorny added a comment.
Shamelessly increased overhead by checking one more path as requested by Kamil.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58592/new/
https://reviews.llvm.org/D58592
Files:
clang/lib/Driver/ToolChains/NetBSD.cpp
Index: clang/lib/Driver/ToolChains/NetBSD.cpp
===
--- clang/lib/Driver/ToolChains/NetBSD.cpp
+++ clang/lib/Driver/ToolChains/NetBSD.cpp
@@ -16,6 +16,7 @@
#include "clang/Driver/Options.h"
#include "clang/Driver/SanitizerArgs.h"
#include "llvm/Option/ArgList.h"
+#include "llvm/Support/VirtualFileSystem.h"
using namespace clang::driver;
using namespace clang::driver::tools;
@@ -422,8 +423,23 @@
void NetBSD::addLibCxxIncludePaths(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ) const {
- addSystemInclude(DriverArgs, CC1Args,
- getDriver().SysRoot + "/usr/include/c++/");
+ const std::string Candidates[] = {
+// directory relative to build tree
+getDriver().Dir + "/../include/c++/v1",
+// system install with full upstream path
+getDriver().SysRoot + "/usr/include/c++/v1",
+// system install from src
+getDriver().SysRoot + "/usr/include/c++",
+ };
+
+ for (const auto : Candidates) {
+if (!getVFS().exists(IncludePath + "/__config"))
+ continue;
+
+// Use the first candidate that looks valid.
+addSystemInclude(DriverArgs, CC1Args, IncludePath);
+return;
+ }
}
void NetBSD::addLibStdCxxIncludePaths(const llvm::opt::ArgList ,
Index: clang/lib/Driver/ToolChains/NetBSD.cpp
===
--- clang/lib/Driver/ToolChains/NetBSD.cpp
+++ clang/lib/Driver/ToolChains/NetBSD.cpp
@@ -16,6 +16,7 @@
#include "clang/Driver/Options.h"
#include "clang/Driver/SanitizerArgs.h"
#include "llvm/Option/ArgList.h"
+#include "llvm/Support/VirtualFileSystem.h"
using namespace clang::driver;
using namespace clang::driver::tools;
@@ -422,8 +423,23 @@
void NetBSD::addLibCxxIncludePaths(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ) const {
- addSystemInclude(DriverArgs, CC1Args,
- getDriver().SysRoot + "/usr/include/c++/");
+ const std::string Candidates[] = {
+// directory relative to build tree
+getDriver().Dir + "/../include/c++/v1",
+// system install with full upstream path
+getDriver().SysRoot + "/usr/include/c++/v1",
+// system install from src
+getDriver().SysRoot + "/usr/include/c++",
+ };
+
+ for (const auto : Candidates) {
+if (!getVFS().exists(IncludePath + "/__config"))
+ continue;
+
+// Use the first candidate that looks valid.
+addSystemInclude(DriverArgs, CC1Args, IncludePath);
+return;
+ }
}
void NetBSD::addLibStdCxxIncludePaths(const llvm::opt::ArgList ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path
joerg added a comment. I'm not in favor of this. It adds overhead for the system compiler and generally makes the logic more complicated. This seems to be another hack around the fact that the driver has no clear notion of "use system runtime" vs "use custom runtime". Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58592/new/ https://reviews.llvm.org/D58592 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path
krytarowski added inline comments. Comment at: clang/lib/Driver/ToolChains/NetBSD.cpp:430 +// system install from src +getDriver().SysRoot + "/usr/include/c++", + }; mgorny wrote: > krytarowski wrote: > > mgorny wrote: > > > krytarowski wrote: > > > > I propose to go for: > > > > > > > > `getDriver().SysRoot + "/usr/include/c++/v1",` with a fallback for > > > > `getDriver().SysRoot + "/usr/include/c++",` > > > > > > > > This innocent customization of paths triggers a lot of headache. > > > > > > > > We should switch system location to `/usr/include/c++/v1` for next > > > > version of LLVM (9.0 as 8.0 is branched and will be released soon). > > > > > > > > This way we will keep compat with legacy paths and new clang. > > > This is already handled by the relative path (when `getDriver().Dir` is > > > `/usr/bin`). > > We still can build newer Clang in pkgsrc on newer base with headers moved > > to `/usr/include/v1/c++` and it will break. > I don't understand. Why would it break? (besides the typo in the path) clang will be installed into `/usr/pkg/bin/clang` and we want to reach the default headers in `/usr/include/c++/v1` (in a setup without installing libc++ in pkgsrc) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58592/new/ https://reviews.llvm.org/D58592 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods
theraven added a comment. Herald added a project: LLVM. After some bisection, it appears that this is the revision that introduced the regression in the GNUstep Objective-C runtime test suite that I reported on the list a few weeks ago. In this is the test (compiled with `-fobjc-runtime=gnustep-2.0 -O3` and an ELF triple): https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m After this change, Early CSE w/ MemorySSA is determining that the second load of `deallocCalled` is redundant. The code goes from: %7 = load i1, i1* @deallocCalled, align 1 br i1 %7, label %8, label %9 ; :8: ; preds = %0 call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8], [15 x i8]* @.str.1, i64 0, i64 0)) #5 unreachable ; :9: ; preds = %0 call void @llvm.objc.autoreleasePoolPop(i8* %1) %10 = load i1, i1* @deallocCalled, align 1 br i1 %10, label %12, label %11 ; :11: ; preds = %9 call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8], [14 x i8]* @.str.2, i64 0, i64 0)) #5 unreachable to: %7 = load i1, i1* @deallocCalled, align 1 br i1 %7, label %8, label %9 ; :8: ; preds = %0 call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8], [15 x i8]* @.str.1, i64 0, i64 0)) #5 unreachable ; :9: ; preds = %0 call void @llvm.objc.autoreleasePoolPop(i8* %1) br i1 %7, label %11, label %10 ; :10: ; preds = %9 call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8], [14 x i8]* @.str.2, i64 0, i64 0)) #5 unreachable Later optimisations then determine that, because the assert does not return, the only possible value for %7 is false and cause the second assert to fire unconditionally. It appears that we are not correctly modelling the side effects of the `llvm.objc.autoreleasePoolPop` intrinsic, but it's not entirely clear why not. The same test compiled for the macos runtime does not appear to exhibit the same behaviour. The previous revision, where we emitted a call to `objc_autoreleasePoolPop` and not the intrinsic worked correctly, but with this change the optimisers are assuming that no globals can be modified across an autorelease pool pop operation (at least, in some situations). Looking at the definition of the intrinsic, I don't see anything wrong, so I still suspect that there is a MemorySSA bug that this has uncovered, rather than anything wrong in this series of commits. Any suggestions as to where to look would be appreciated. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55802/new/ https://reviews.llvm.org/D55802 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path
mgorny marked an inline comment as done. mgorny added inline comments. Comment at: clang/lib/Driver/ToolChains/NetBSD.cpp:430 +// system install from src +getDriver().SysRoot + "/usr/include/c++", + }; krytarowski wrote: > mgorny wrote: > > krytarowski wrote: > > > I propose to go for: > > > > > > `getDriver().SysRoot + "/usr/include/c++/v1",` with a fallback for > > > `getDriver().SysRoot + "/usr/include/c++",` > > > > > > This innocent customization of paths triggers a lot of headache. > > > > > > We should switch system location to `/usr/include/c++/v1` for next > > > version of LLVM (9.0 as 8.0 is branched and will be released soon). > > > > > > This way we will keep compat with legacy paths and new clang. > > This is already handled by the relative path (when `getDriver().Dir` is > > `/usr/bin`). > We still can build newer Clang in pkgsrc on newer base with headers moved to > `/usr/include/v1/c++` and it will break. I don't understand. Why would it break? (besides the typo in the path) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58592/new/ https://reviews.llvm.org/D58592 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path
krytarowski added inline comments. Comment at: clang/lib/Driver/ToolChains/NetBSD.cpp:430 +// system install from src +getDriver().SysRoot + "/usr/include/c++", + }; mgorny wrote: > krytarowski wrote: > > I propose to go for: > > > > `getDriver().SysRoot + "/usr/include/c++/v1",` with a fallback for > > `getDriver().SysRoot + "/usr/include/c++",` > > > > This innocent customization of paths triggers a lot of headache. > > > > We should switch system location to `/usr/include/c++/v1` for next version > > of LLVM (9.0 as 8.0 is branched and will be released soon). > > > > This way we will keep compat with legacy paths and new clang. > This is already handled by the relative path (when `getDriver().Dir` is > `/usr/bin`). We still can build newer Clang in pkgsrc on newer base with headers moved to `/usr/include/v1/c++` and it will break. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58592/new/ https://reviews.llvm.org/D58592 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path
mgorny marked an inline comment as done. mgorny added inline comments. Comment at: clang/lib/Driver/ToolChains/NetBSD.cpp:430 +// system install from src +getDriver().SysRoot + "/usr/include/c++", + }; krytarowski wrote: > I propose to go for: > > `getDriver().SysRoot + "/usr/include/c++/v1",` with a fallback for > `getDriver().SysRoot + "/usr/include/c++",` > > This innocent customization of paths triggers a lot of headache. > > We should switch system location to `/usr/include/c++/v1` for next version of > LLVM (9.0 as 8.0 is branched and will be released soon). > > This way we will keep compat with legacy paths and new clang. This is already handled by the relative path (when `getDriver().Dir` is `/usr/bin`). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58592/new/ https://reviews.llvm.org/D58592 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path
krytarowski added inline comments. Comment at: clang/lib/Driver/ToolChains/NetBSD.cpp:430 +// system install from src +getDriver().SysRoot + "/usr/include/c++", + }; I propose to go for: `getDriver().SysRoot + "/usr/include/c++/v1",` with a fallback for `getDriver().SysRoot + "/usr/include/c++",` This innocent customization of paths triggers a lot of headache. We should switch system location to `/usr/include/c++/v1` for next version of LLVM (9.0 as 8.0 is branched and will be released soon). This way we will keep compat with legacy paths and new clang. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58592/new/ https://reviews.llvm.org/D58592 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58592: [clang] [ToolChains/NetBSD] Support relative libc++ header path
mgorny created this revision.
mgorny added reviewers: krytarowski, joerg, chandlerc, eugenis.
Herald added a reviewer: EricWF.
Herald added a project: clang.
Support locating the libc++ header files relatively to the clang
executable, in addition to the default system path. This is meant
to cover two use cases: running just-built clang from the install
directory, and running installed clang from non-standard location
(e.g. /usr/local).
This is the first step towards ensuring that tests of more LLVM projects
can work out-of-the-box within the build tree, and use the correct set
of headers (rather than e.g. mixing just-built clang+libcxx with system
install of libcxx). It avoids requiring the user to hack around missing
include paths, or LLVM build system to replicate system-specific C++
library defaults in order to append appropriate paths implicitly.
Repository:
rC Clang
https://reviews.llvm.org/D58592
Files:
clang/lib/Driver/ToolChains/NetBSD.cpp
Index: clang/lib/Driver/ToolChains/NetBSD.cpp
===
--- clang/lib/Driver/ToolChains/NetBSD.cpp
+++ clang/lib/Driver/ToolChains/NetBSD.cpp
@@ -16,6 +16,7 @@
#include "clang/Driver/Options.h"
#include "clang/Driver/SanitizerArgs.h"
#include "llvm/Option/ArgList.h"
+#include "llvm/Support/VirtualFileSystem.h"
using namespace clang::driver;
using namespace clang::driver::tools;
@@ -422,8 +423,21 @@
void NetBSD::addLibCxxIncludePaths(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ) const {
- addSystemInclude(DriverArgs, CC1Args,
- getDriver().SysRoot + "/usr/include/c++/");
+ const std::string Candidates[] = {
+// directory relative to build tree
+getDriver().Dir + "/../include/c++/v1",
+// system install from src
+getDriver().SysRoot + "/usr/include/c++",
+ };
+
+ for (const auto : Candidates) {
+if (!getVFS().exists(IncludePath + "/__config"))
+ continue;
+
+// Use the first candidate that looks valid.
+addSystemInclude(DriverArgs, CC1Args, IncludePath);
+return;
+ }
}
void NetBSD::addLibStdCxxIncludePaths(const llvm::opt::ArgList ,
Index: clang/lib/Driver/ToolChains/NetBSD.cpp
===
--- clang/lib/Driver/ToolChains/NetBSD.cpp
+++ clang/lib/Driver/ToolChains/NetBSD.cpp
@@ -16,6 +16,7 @@
#include "clang/Driver/Options.h"
#include "clang/Driver/SanitizerArgs.h"
#include "llvm/Option/ArgList.h"
+#include "llvm/Support/VirtualFileSystem.h"
using namespace clang::driver;
using namespace clang::driver::tools;
@@ -422,8 +423,21 @@
void NetBSD::addLibCxxIncludePaths(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ) const {
- addSystemInclude(DriverArgs, CC1Args,
- getDriver().SysRoot + "/usr/include/c++/");
+ const std::string Candidates[] = {
+// directory relative to build tree
+getDriver().Dir + "/../include/c++/v1",
+// system install from src
+getDriver().SysRoot + "/usr/include/c++",
+ };
+
+ for (const auto : Candidates) {
+if (!getVFS().exists(IncludePath + "/__config"))
+ continue;
+
+// Use the first candidate that looks valid.
+addSystemInclude(DriverArgs, CC1Args, IncludePath);
+return;
+ }
}
void NetBSD::addLibStdCxxIncludePaths(const llvm::opt::ArgList ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
