[PATCH] D52401: Remove redundant null pointer check in operator delete
lichray added a comment. In https://reviews.llvm.org/D52401#1250551, @MaskRay wrote: > `__free_hook` (defaults to NULL) is a user-supplied hook > (https://www.gnu.org/software/libc/manual/html_node/Hooks-for-Malloc.html). Very interesting, that means if we don't apply this patch, we essentially breaks glic `__free_hook`, because that one expects to be able to observe null pointers. Repository: rL LLVM https://reviews.llvm.org/D52401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52401: Remove redundant null pointer check in operator delete
MaskRay added a comment. > I seem to recall a problem with that I would like to know if your impression came from the common PWN technique when the attacker found a heap buffer overflow :) Repository: rL LLVM https://reviews.llvm.org/D52401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52401: Remove redundant null pointer check in operator delete
MaskRay added a comment.
I just checked an extremely old version of glibc fetched from
https://ftp.gnu.org/pub/gnu/glibc/
glibc-2.0.1.tar.gz 1997-02-04 03:003.7M
`malloc/malloc.c`
#if __STD_C
void fREe(Void_t* mem)
#else
void fREe(mem) Void_t* mem;
#endif
{
arena *ar_ptr;
mchunkptr p; /* chunk corresponding to mem */
#if defined(_LIBC) || defined(MALLOC_HOOKS)
if (__free_hook != NULL) {
(*__free_hook)(mem);
return;
}
#endif
if (mem == 0) /* free(0) has no effect */
return;
`__free_hook` (defaults to NULL) is a user-supplied hook
(https://www.gnu.org/software/libc/manual/html_node/Hooks-for-Malloc.html). If
this failed for `operator delete`, it would mean various other `free(NULL)`
would also fail.
Repository:
rL LLVM
https://reviews.llvm.org/D52401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52696: Update ifunc attribute support documentation
joerg added a comment. Yeah, I would restrict it to just mention that it depends on the target, link time editor and runtime linker. Even the concrete feature set on Linux changes with glibc versions. Repository: rL LLVM https://reviews.llvm.org/D52696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52696: Update ifunc attribute support documentation
emaste added a comment. Maybe "available for some architectures in at least..."? Or maybe we shouldn't bother trying to list versions, and mention it is dependent on CPU arch, linker, and rtld? Repository: rL LLVM https://reviews.llvm.org/D52696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52401: Remove redundant null pointer check in operator delete
mclow.lists added a comment. I suspect it's fine, but I need to check some stuff on old versions of glibc (I seem to recall a problem with that). Repository: rL LLVM https://reviews.llvm.org/D52401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51633: [ASTImporter] Added error handling for AST import.
a_sidorin added a comment.
Herald added a subscriber: Szelethus.
Hi Balasz,
It's going to take some time to review the whole patch.
Comment at: lib/AST/ASTImporter.cpp:194
+ // FIXME: This should be the final code.
+ //auto ToOrErr = Importer.Import(From);
+ //if (ToOrErr) {
Do I understand correctly that we have to use the upper variant because
ASTImporter API is unchanged?
Comment at: lib/AST/ASTImporter.cpp:417
-bool ImportDefinition(RecordDecl *From, RecordDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
-bool ImportDefinition(VarDecl *From, VarDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
-bool ImportDefinition(EnumDecl *From, EnumDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
-bool ImportDefinition(ObjCInterfaceDecl *From, ObjCInterfaceDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
-bool ImportDefinition(ObjCProtocolDecl *From, ObjCProtocolDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
-TemplateParameterList *ImportTemplateParameterList(
+Error ImportDefinition(
+RecordDecl *From, RecordDecl *To,
balazske wrote:
> a_sidorin wrote:
> > The changes for argument indentation look redundant. Is it a result of
> > clang-format?
> clang-format aligns start of new argument lines to the `(` character above.
> Indentations are not always consistent in this file, I can reformat the whole
> file with clang-format.
> (But I do not like the formatting like
> ```
> Error ImportDeclParts(NamedDecl *D, DeclContext *, DeclContext
> *,
> DeclarationName , NamedDecl *,
> SourceLocation );
> ```
> if there is a `LongNamedClass::LongReturnType
> LongNamedClass::LongNamedFunction`.)
>
No need to reformat the whole file. As I see, the indentation changes are not
breaking and too wide to roll them back so we can leave them as-is.
Comment at: lib/AST/ASTImporter.cpp:1087
- QualType ClassType = Importer.Import(QualType(T->getClass(), 0));
- return Importer.getToContext().getMemberPointerType(ToPointeeType,
- ClassType.getTypePtr());
+ ExpectedType ClassTypeOrErr = import(QualType(T->getClass(), 0));
+ if (!ClassTypeOrErr)
Nice catch!
Comment at: lib/AST/ASTImporter.cpp:1658
}
+ llvm::SmallVector ImportedDecls;
+ for (auto *From : FromDC->decls()) {
Space before '*' is needed.
Comment at: lib/AST/ASTImporter.cpp:1663
+ // Ignore the error, continue with next Decl.
+ consumeError(ImportedOrErr.takeError());
+ }
Silent fail doesn't look correct in case of structures. Should we add a FIXME?
Comment at: lib/AST/ASTImporter.cpp:1953
- return false;
+Expected
+ASTNodeImporter::ImportTemplateArgument(const TemplateArgument ) {
Should we add a FIXME for removing this method?
Comment at: lib/AST/ASTImporter.cpp:2266
+ ToD, D, Importer.getToContext(), DC, ToNamespaceLoc, ToAliasLoc,
+ ToIdentifier,ToQualifierLoc, ToTargetNameLoc, ToNamespace))
return ToD;
Space after comma is lost.
Comment at: lib/AST/ASTImporter.cpp:2656
}
+ if (IsFriendTemplate)
+continue;
This move looks like an additional functional change. Could you please explain
the reason?
Comment at: lib/AST/ASTImporter.cpp:2683
+continue;
+ } else if (isa(Found))
+continue;
Same here.
Comment at: lib/AST/ASTImporter.cpp:2706
CXXRecordDecl *D2CXX = nullptr;
-if (auto *DCXX = dyn_cast(D)) {
+if (auto *DCXX = llvm::dyn_cast(D)) {
if (DCXX->isLambda()) {
We usually don't qualify casts.
Comment at: lib/AST/ASTImporter.cpp:4136
return ToProto;
}
// Stopped here.
Repository:
rC Clang
https://reviews.llvm.org/D51633
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52445: [Index] Use locations to uniquify function-scope BindingDecl USR
MaskRay added a comment. Ping Repository: rC Clang https://reviews.llvm.org/D52445 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52705: First-pass at ARM hard/soft-float multilib driver (NOT WORKING YET)
kelledin created this revision.
Herald added a reviewer: javed.absar.
Herald added subscribers: cfe-commits, chrib, kristof.beyls.
This is my first crack at implementing working ARM EABI multilib support
(where multilib support is between hard/soft float ONLY, not between 32-bit
and 64-bit). This is currently NOT suitable for merging; I'm only posting
it for guidance as to what exactly I'm missing. Specifically, for the
default-hardfloat target (arm-linux-gnueabihf), clang selects the correct
GCC suffix (/sf vs /hf) but consistently selects /usr/lib/arm-linux-gnueabihf
as the library directory, even if soft-float flags are passed. This leaves
binutils trying to mix hard-float/soft-float objects, which of course fails
miserably. AIUI the multilib driver *should* trigger the selection of
/usr/lib/arm-linux-gnueabi instead.
I can produce verbose output from clang's behavior upon request. Be aware
that this changeset depends on https://reviews.llvm.org/D52149 for
compiler-rt.
Repository:
rC Clang
https://reviews.llvm.org/D52705
Files:
lib/Driver/ToolChains/Arch/ARM.cpp
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Linux.cpp
Index: lib/Driver/ToolChains/Linux.cpp
===
--- lib/Driver/ToolChains/Linux.cpp
+++ lib/Driver/ToolChains/Linux.cpp
@@ -533,8 +533,9 @@
case llvm::Triple::thumb:
case llvm::Triple::armeb:
case llvm::Triple::thumbeb: {
+// NOTE: If no float-ABI selector flags are in use, getARMFloatABI()
+// will fall back on determining ABI by Triple.getEnvironment().
const bool HF =
-Triple.getEnvironment() == llvm::Triple::GNUEABIHF ||
tools::arm::getARMFloatABI(*this, Args) == tools::arm::FloatABI::Hard;
LibDir = "lib";
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -808,9 +808,22 @@
if (!A)
return false;
+ // ARM GNUEABI allows a "softfp" ABI (compatible with soft-float calling
+ // conventions, but can still generate FPU instructions inside functions).
return A->getOption().matches(options::OPT_msoft_float) ||
(A->getOption().matches(options::OPT_mfloat_abi_EQ) &&
- A->getValue() == StringRef("soft"));
+ (A->getValue() == StringRef("soft") || A->getValue() == StringRef("softfp")));
+}
+
+static bool isHardFloatABI(const ArgList ) {
+ Arg *A = Args.getLastArg(options::OPT_msoft_float, options::OPT_mhard_float,
+ options::OPT_mfloat_abi_EQ);
+ if (!A)
+return false;
+
+ return A->getOption().matches(options::OPT_mhard_float) ||
+ (A->getOption().matches(options::OPT_mfloat_abi_EQ) &&
+ A->getValue() == StringRef("hard"));
}
/// \p Flag must be a flag accepted by the driver with its leading '-' removed,
@@ -1425,6 +1438,75 @@
Result.Multilibs = RISCVMultilibs;
}
+static bool findArmEABIMultilibs(const Driver ,
+ const llvm::Triple ,
+ StringRef Path, const ArgList ,
+ DetectedMultilibs ) {
+ // Find multilibs with subdirectories like hf (for hard-float) or sf (for
+ // soft-float). gcc also allows for "-mfloat-abi=softfp": ABI-equivalent
+ // to soft-float, but can still generate FPU instuctions inside functions.
+ // softfp usually has the same multilib subdirectory as soft-float.
+ Multilib Default;
+ bool DefaultHardFloat = TargetTriple.isHardFloatEABI();
+ llvm::Triple AltTriple(DefaultHardFloat ?
+ TargetTriple.getSoftFloatArchVariant() :
+ TargetTriple.getHardFloatArchVariant());
+ // We assume the Debian libdir/includedir arrangement for armel/armhf,
+ // since Debian and its descendents are apparently the only common Linux
+ // distros that have anything resembling multilib support for this scenario.
+ // SuSE and RedHat seem to stick with hard-float only and no libdir suffix.
+ // TODO: this (and a lot of other code here) could be generalized via the
+ // output of "gcc --print-multi-os-dir".
+ Multilib ArmHFMultilib = makeMultilib("/hf")
+ .osSuffix("/" + TargetTriple.getHardFloatArchVariant().str())
+ .includeSuffix("/" + TargetTriple.getHardFloatArchVariant().str())
+ .flag("+mhard-float")
+ .flag("+mfloat-abi=hard")
+ .flag("-msoft-float")
+ .flag("-mfloat-abi=soft")
+ .flag("-mfloat-abi=softfp");
+ Multilib ArmSFMultilib = makeMultilib("/sf")
+ .osSuffix("/" + TargetTriple.getSoftFloatArchVariant().str())
+ .includeSuffix("/" +
r343425 - Use the container form llvm::sort(C, ...)
Author: maskray
Date: Sun Sep 30 14:41:11 2018
New Revision: 343425
URL: http://llvm.org/viewvc/llvm-project?rev=343425=rev
Log:
Use the container form llvm::sort(C, ...)
There are a few leftovers of rC343147 that are not (\w+)\.begin but in
the form of ([-[:alnum:]>.]+)\.begin or spanning two lines. Change them
to use the container form in this commit. The 12 occurrences have been
inspected manually for safety.
Modified:
cfe/trunk/include/clang/Serialization/ContinuousRangeMap.h
cfe/trunk/lib/AST/ASTContext.cpp
cfe/trunk/lib/AST/VTableBuilder.cpp
cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp
cfe/trunk/lib/Basic/Targets.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
cfe/trunk/utils/TableGen/ClangDiagnosticsEmitter.cpp
Modified: cfe/trunk/include/clang/Serialization/ContinuousRangeMap.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ContinuousRangeMap.h?rev=343425=343424=343425=diff
==
--- cfe/trunk/include/clang/Serialization/ContinuousRangeMap.h (original)
+++ cfe/trunk/include/clang/Serialization/ContinuousRangeMap.h Sun Sep 30
14:41:11 2018
@@ -118,7 +118,7 @@ public:
Builder =(const Builder&) = delete;
~Builder() {
- llvm::sort(Self.Rep.begin(), Self.Rep.end(), Compare());
+ llvm::sort(Self.Rep, Compare());
std::unique(Self.Rep.begin(), Self.Rep.end(),
[](const_reference A, const_reference B) {
// FIXME: we should not allow any duplicate keys, but there are a lot
of
Modified: cfe/trunk/lib/AST/ASTContext.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=343425=343424=343425=diff
==
--- cfe/trunk/lib/AST/ASTContext.cpp (original)
+++ cfe/trunk/lib/AST/ASTContext.cpp Sun Sep 30 14:41:11 2018
@@ -2295,12 +2295,11 @@ structHasUniqueObjectRepresentations(con
}
}
-llvm::sort(
-Bases.begin(), Bases.end(), [&](const std::pair ,
-const std::pair )
{
- return Layout.getBaseClassOffset(L.first->getAsCXXRecordDecl()) <
- Layout.getBaseClassOffset(R.first->getAsCXXRecordDecl());
-});
+llvm::sort(Bases, [&](const std::pair ,
+ const std::pair ) {
+ return Layout.getBaseClassOffset(L.first->getAsCXXRecordDecl()) <
+ Layout.getBaseClassOffset(R.first->getAsCXXRecordDecl());
+});
for (const auto Base : Bases) {
int64_t BaseOffset = Context.toBits(
Modified: cfe/trunk/lib/AST/VTableBuilder.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/VTableBuilder.cpp?rev=343425=343424=343425=diff
==
--- cfe/trunk/lib/AST/VTableBuilder.cpp (original)
+++ cfe/trunk/lib/AST/VTableBuilder.cpp Sun Sep 30 14:41:11 2018
@@ -2205,13 +2205,12 @@ VTableLayout::VTableLayout(ArrayRefVTableIndices = OwningArrayRef(VTableIndices);
- llvm::sort(this->VTableThunks.begin(), this->VTableThunks.end(),
- [](const VTableLayout::VTableThunkTy ,
-const VTableLayout::VTableThunkTy ) {
- assert((LHS.first != RHS.first || LHS.second == RHS.second) &&
- "Different thunks should have unique indices!");
- return LHS.first < RHS.first;
-});
+ llvm::sort(this->VTableThunks, [](const VTableLayout::VTableThunkTy ,
+const VTableLayout::VTableThunkTy ) {
+assert((LHS.first != RHS.first || LHS.second == RHS.second) &&
+ "Different thunks should have unique indices!");
+return LHS.first < RHS.first;
+ });
}
VTableLayout::~VTableLayout() { }
Modified: cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp?rev=343425=343424=343425=diff
==
--- cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp (original)
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Parser.cpp Sun Sep 30 14:41:11 2018
@@ -645,12 +645,12 @@ Parser::completeExpression(StringRef Cod
P.parseExpressionImpl();
// Sort by specificity, then by name.
- llvm::sort(P.Completions.begin(), P.Completions.end(),
+ llvm::sort(P.Completions,
[](const MatcherCompletion , const MatcherCompletion ) {
-if (A.Specificity != B.Specificity)
- return A.Specificity > B.Specificity;
-return A.TypedText < B.TypedText;
- });
+ if (A.Specificity != B.Specificity)
+ return A.Specificity > B.Specificity;
+ return A.TypedText < B.TypedText;
+ });
return P.Completions;
}
[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.
EricWF updated this revision to Diff 167664.
EricWF added a comment.
Build, cache and substitute the `SourceLocExpr` for a `StringLiteral` during
constant evaluation. This is instead of the changes to `LValueBase`which stored
the string value and type in addition to the original `SourceLocExpr`.
Currently only `SourceLocExpr` uses the `StringLiteral` cache in `ASTContext`.
`PredefinedExpr` could be made to use it at the expense of losing the
`SourceLocation` information the `StringLiteral` caches.
@rsmith How does this look now?
I'll follow up with a separate `__builtin_SOURCE_LOCATION()` patch which
returns a single relocatable object containing the file, func, line and column.
@rsmith Any pointers on how to implement that?
https://reviews.llvm.org/D37035
Files:
docs/LanguageExtensions.rst
include/clang/AST/ASTContext.h
include/clang/AST/CurrentSourceLocExprScope.h
include/clang/AST/Expr.h
include/clang/AST/ExprCXX.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/AST/Stmt.h
include/clang/Basic/StmtNodes.td
include/clang/Basic/TokenKinds.def
include/clang/Sema/Sema.h
include/clang/Serialization/ASTBitCodes.h
lib/AST/ASTContext.cpp
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprCXX.cpp
lib/AST/ExprClassification.cpp
lib/AST/ExprConstant.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/StmtPrinter.cpp
lib/AST/StmtProfile.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprAgg.cpp
lib/CodeGen/CGExprComplex.cpp
lib/CodeGen/CGExprConstant.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
lib/Parse/ParseExpr.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExceptionSpec.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/TreeTransform.h
lib/Serialization/ASTReaderStmt.cpp
lib/Serialization/ASTWriterStmt.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/CodeGenCXX/builtin-source-location.cpp
test/CodeGenCXX/builtin_FUNCTION.cpp
test/CodeGenCXX/builtin_LINE.cpp
test/CodeGenCXX/debug-info-line.cpp
test/Parser/builtin_source_location.c
test/Sema/source_location.c
test/SemaCXX/Inputs/source-location-file.h
test/SemaCXX/source_location.cpp
Index: test/SemaCXX/source_location.cpp
===
--- /dev/null
+++ test/SemaCXX/source_location.cpp
@@ -0,0 +1,590 @@
+// RUN: %clang_cc1 -std=c++1z -fcxx-exceptions -fexceptions -verify %s
+// expected-no-diagnostics
+
+#define assert(...) ((__VA_ARGS__) ? ((void)0) : throw 42)
+#define CURRENT_FROM_MACRO() SL::current()
+#define FORWARD(...) __VA_ARGS__
+
+template
+struct Printer;
+
+namespace std {
+namespace experimental {
+struct source_location {
+private:
+ unsigned int __m_line = 0;
+ unsigned int __m_col = 0;
+ const char *__m_file = nullptr;
+ const char *__m_func = nullptr;
+public:
+ static constexpr source_location current(
+ const char *__file = __builtin_FILE(),
+ const char *__func = __builtin_FUNCTION(),
+ unsigned int __line = __builtin_LINE(),
+ unsigned int __col = __builtin_COLUMN()) noexcept {
+source_location __loc;
+__loc.__m_line = __line;
+__loc.__m_col = __col;
+__loc.__m_file = __file;
+__loc.__m_func = __func;
+return __loc;
+ }
+ constexpr source_location() = default;
+ constexpr source_location(source_location const &) = default;
+ constexpr unsigned int line() const noexcept { return __m_line; }
+ constexpr unsigned int column() const noexcept { return __m_col; }
+ constexpr const char *file() const noexcept { return __m_file; }
+ constexpr const char *function() const noexcept { return __m_func; }
+};
+} // namespace experimental
+} // namespace std
+
+using SL = std::experimental::source_location;
+
+#include "Inputs/source-location-file.h"
+namespace SLF = source_location_file;
+
+constexpr bool is_equal(const char *LHS, const char *RHS) {
+ while (*LHS != 0 && *RHS != 0) {
+if (*LHS != *RHS)
+ return false;
+++LHS;
+++RHS;
+ }
+ return *LHS == 0 && *RHS == 0;
+}
+
+template
+constexpr T identity(T t) {
+ return t;
+}
+
+template
+struct Pair {
+ T first;
+ U second;
+};
+
+template
+constexpr bool is_same = false;
+template
+constexpr bool is_same = true;
+
+// test types
+static_assert(is_same);
+static_assert(is_same);
+static_assert(is_same);
+static_assert(is_same);
+
+// test noexcept
+static_assert(noexcept(__builtin_LINE()));
+static_assert(noexcept(__builtin_COLUMN()));
+static_assert(noexcept(__builtin_FILE()));
+static_assert(noexcept(__builtin_FUNCTION()));
+
+//===--===//
+//__builtin_LINE()
+//===--===//
+
+namespace test_line {
+static_assert(SL::current().line() == __LINE__);
+static_assert(SL::current().line() == CURRENT_FROM_MACRO().line());
+
+static constexpr SL GlobalS = SL::current();
+
+static_assert(GlobalS.line() == __LINE__ - 2);
+
+//
[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.
EricWF marked 10 inline comments as done.
EricWF added inline comments.
Comment at: include/clang/AST/APValue.h:66-73
+LValueBase(const Expr *E, const char *StrVal,
+ const ConstantArrayType *ArrTy);
template
-LValueBase(T P, unsigned I = 0, unsigned V = 0)
-: Ptr(P), CallIndex(I), Version(V) {}
+LValueBase(T P, unsigned I = 0, unsigned V = 0) : Ptr(P), NormalData{I, V}
{
+ assert(getBaseKind() == BK_Normal &&
+ "cannot create string base with this constructor");
rsmith wrote:
> This seems like a slightly dangerous overload set (eg, what does
> `LValueBase(P, 0, 0);` call when `P` is of type `Expr*`?)
Removed in place of the string literal caching implementation.
Comment at: include/clang/AST/APValue.h:143-146
+union {
+ NormalLValueData NormalData;
+ GlobalStringData StrData;
+};
rsmith wrote:
> This makes `LValueBase` 8 bytes larger (on 64-bit targets) to support an
> uncommon case. Does `APValue` get larger too, or is its size dominated by
> something else? If needed, we could avoid the size increase by separately
> allocating the `GlobalStringData` and storing a pointer to it here -- and
> maybe something like a `GlobalStringData*` is what we should be caching on
> the `ASTContext` instead of a `const char*`?
>
> However, as you mentioned in a prior comment:
>
> > An alternative solution would be to create a `StringLiteral`, cache it in
> > `ASTContext` for reuse, and return the new expression. Would this require
> > serializing the `ASTContext` cache? Would this be a better solution?
>
> Yes, I think that'd be a better and cleaner solution. We shouldn't need to
> serialize the `ASTContext` cache, because we never serialize `APValue`s. (And
> if we ever start doing so, serialization of lvalues denoting `Expr*`s becomes
> a hard problem in general, not only in this case.)
>
> Naturally, you'll need to cache the strings used by `__builtin_FILE` in
> addition to the current cache for strings used by `__builtin_FUNCTION`, but
> on the plus side you should be able to reuse the existing code that generates
> `StringLiteral` objects for `PredefinedExpr`s (and maybe you could even share
> a cache between the new builtins and the `PredefinedExpr` handling?)
> Yes, I think that'd be a better and cleaner solution. We shouldn't need to
> serialize the ASTContext cache, because we never serialize APValues. (And if
> we ever start doing so, serialization of lvalues denoting Expr*s becomes a
> hard problem in general, not only in this case.)
SGTM. I've implemented this instead.
> on the plus side you should be able to reuse the existing code that generates
> StringLiteral objects for PredefinedExprs (and maybe you could even share a
> cache between the new builtins and the PredefinedExpr handling?)
The only issue with caching, and in particular adding it to `PredefinedExpr`,
is that the `StringLiteral` no longer has meaningful location information. Is
this OK?
https://reviews.llvm.org/D37035
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)
lebedev.ri added inline comments. Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:660-678 - // Check if the range is entirely contained within a macro argument. - SourceLocation MacroArgExpansionStartForRangeBegin; - SourceLocation MacroArgExpansionStartForRangeEnd; - bool RangeIsEntirelyWithinMacroArgument = - SourceMgr && - SourceMgr->isMacroArgExpansion(Range.getBegin(), - ) && @JonasToth that is what this code did. From a quick look i'm not sure what it would decide without the source manager, so i simply refactored it as a function, and kept the semantics. Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165 + + diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case") + << LiteralType::Name << S.OldSuffix JonasToth wrote: > lebedev.ri wrote: > > lebedev.ri wrote: > > > JonasToth wrote: > > > > lebedev.ri wrote: > > > > > JonasToth wrote: > > > > > > JonasToth wrote: > > > > > > > Lets move this `diag` into the true branch of the if stmt and > > > > > > > drop the ´else`. > > > > > > I think the warning should point at the suffix, which can be > > > > > > retrieved from the replacement range. Then you don't need to > > > > > > include the suffix itself in the diagnostic > > > > > If the warnings are aggregated (i.e. not raw `make` dump), with only > > > > > the first line shown, the suffix will still be invisible. > > > > > > > > > > Regarding the pointer direction, i'm not sure. > > > > > For now i have reworded the diag to justify pointing at the literal > > > > > itself. > > > > I don't understand that. The warning message does include the source > > > > location that would be clearly on the literal suffix and the warning > > > > without the suffix printed is clear as well. Having this slightly > > > > simpler diagnostic would simplify the code significantly. > > > *location*. which will still be a location, if only the first line of the > > > warning is displayed. > > > > > > We won't even win all that much. > > > Sure, we will return `Optional`, but we will still need to do > > > all that internal stuff to find the old suffix, uppercase it, compare > > > them, etc. > > > > > > And we lose a very useful ability to check what it considered to be the > > > old suffix in the tests. > > > > > > It is basically the same question as in D51949. > > > If you insist, sure, i can do that, but i *really* do believe this is > > > **WRONG**. > > And one more problem here, where do we point if we don't have a fix-it to > > get the suffix location from? > That point is valid, but if we dont have a suffix location, there is no > suffix? > I dont insist :) See `test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp`, `horrible_macros()` I guess if we point at the suffix, we will also get less useful `expanded from` messages. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52670 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)
lebedev.ri updated this revision to Diff 167661.
lebedev.ri marked 5 inline comments as done.
lebedev.ri added a comment.
Address review notes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
Files:
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/cert/CMakeLists.txt
clang-tidy/hicpp/HICPPTidyModule.cpp
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/IdentifierNamingCheck.cpp
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
clang-tidy/readability/UppercaseLiteralSuffixCheck.h
clang-tidy/utils/ASTUtils.cpp
clang-tidy/utils/ASTUtils.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cert-dcl16-c.rst
docs/clang-tidy/checks/hicpp-uppercase-literal-suffix.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst
test/clang-tidy/readability-uppercase-literal-suffix-floating-point.cpp
test/clang-tidy/readability-uppercase-literal-suffix-hexadecimal-floating-point.cpp
test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp
test/clang-tidy/readability-uppercase-literal-suffix-integer.cpp
test/clang-tidy/readability-uppercase-literal-suffix.h
Index: test/clang-tidy/readability-uppercase-literal-suffix.h
===
--- /dev/null
+++ test/clang-tidy/readability-uppercase-literal-suffix.h
@@ -0,0 +1,16 @@
+template
+struct integral_constant {
+ static constexpr T value = v;
+ typedef T value_type;
+ typedef integral_constant type; // using injected-class-name
+ constexpr operator value_type() const noexcept { return value; }
+};
+
+using false_type = integral_constant;
+using true_type = integral_constant;
+
+template
+struct is_same : false_type {};
+
+template
+struct is_same : true_type {};
Index: test/clang-tidy/readability-uppercase-literal-suffix-integer.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-uppercase-literal-suffix-integer.cpp
@@ -0,0 +1,213 @@
+// RUN: %check_clang_tidy %s readability-uppercase-literal-suffix %t -- -- -I %S
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: clang-tidy %t.cpp -checks='-*,readability-uppercase-literal-suffix' -fix -- -I %S
+// RUN: clang-tidy %t.cpp -checks='-*,readability-uppercase-literal-suffix' -warnings-as-errors='-*,readability-uppercase-literal-suffix' -- -I %S
+
+#include "readability-uppercase-literal-suffix.h"
+
+void integer_suffix() {
+ static constexpr auto v0 = __LINE__; // synthetic
+ static_assert(v0 == 9 || v0 == 5, "");
+
+ static constexpr auto v1 = __cplusplus; // synthetic, long
+
+ static constexpr auto v2 = 1; // no literal
+ static_assert(is_same::value, "");
+ static_assert(v2 == 1, "");
+
+ // Unsigned
+
+ static constexpr auto v3 = 1u;
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: integer literal has suffix 'u', which is not upper-case
+ // CHECK-MESSAGES-NEXT: static constexpr auto v3 = 1u;
+ // CHECK-MESSAGES-NEXT: ^~
+ // CHECK-MESSAGES-NEXT: {{^ *}}U{{$}}
+ // CHECK-FIXES: static constexpr auto v3 = 1U;
+ static_assert(is_same::value, "");
+ static_assert(v3 == 1, "");
+
+ static constexpr auto v4 = 1U; // OK.
+ static_assert(is_same::value, "");
+ static_assert(v4 == 1, "");
+
+ // Long
+
+ static constexpr auto v5 = 1l;
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: integer literal has suffix 'l', which is not upper-case
+ // CHECK-MESSAGES-NEXT: static constexpr auto v5 = 1l;
+ // CHECK-MESSAGES-NEXT: ^~
+ // CHECK-MESSAGES-NEXT: {{^ *}}L{{$}}
+ // CHECK-FIXES: static constexpr auto v5 = 1L;
+ static_assert(is_same::value, "");
+ static_assert(v5 == 1, "");
+
+ static constexpr auto v6 = 1L; // OK.
+ static_assert(is_same::value, "");
+ static_assert(v6 == 1, "");
+
+ // Long Long
+
+ static constexpr auto v7 = 1ll;
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: integer literal has suffix 'll', which is not upper-case
+ // CHECK-MESSAGES-NEXT: static constexpr auto v7 = 1ll;
+ // CHECK-MESSAGES-NEXT: ^~~
+ // CHECK-MESSAGES-NEXT: {{^ *}}LL{{$}}
+ // CHECK-FIXES: static constexpr auto v7 = 1LL;
+ static_assert(is_same::value, "");
+ static_assert(v7 == 1, "");
+
+ static constexpr auto v8 = 1LL; // OK.
+ static_assert(is_same::value, "");
+ static_assert(v8 == 1, "");
+
+ // Unsigned Long
+
+ static constexpr auto v9 = 1ul;
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: integer literal has suffix 'ul', which is not upper-case
+ // CHECK-MESSAGES-NEXT: static constexpr auto v9 = 1ul;
+ // CHECK-MESSAGES-NEXT: ^~~
+ // CHECK-MESSAGES-NEXT: {{^ *}}UL{{$}}
+ // CHECK-FIXES: static constexpr auto v9 = 1UL;
+ static_assert(is_same::value, "");
+ static_assert(v9 == 1, "");
+
+ static constexpr auto v10 = 1uL;
+ // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: integer literal has suffix 'uL', which is not upper-case
+ // CHECK-MESSAGES-NEXT: static constexpr auto v10 = 1uL;
+
[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)
JonasToth added inline comments.
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165
+
+ diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case")
+ << LiteralType::Name << S.OldSuffix
lebedev.ri wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > lebedev.ri wrote:
> > > > JonasToth wrote:
> > > > > JonasToth wrote:
> > > > > > Lets move this `diag` into the true branch of the if stmt and drop
> > > > > > the ´else`.
> > > > > I think the warning should point at the suffix, which can be
> > > > > retrieved from the replacement range. Then you don't need to include
> > > > > the suffix itself in the diagnostic
> > > > If the warnings are aggregated (i.e. not raw `make` dump), with only
> > > > the first line shown, the suffix will still be invisible.
> > > >
> > > > Regarding the pointer direction, i'm not sure.
> > > > For now i have reworded the diag to justify pointing at the literal
> > > > itself.
> > > I don't understand that. The warning message does include the source
> > > location that would be clearly on the literal suffix and the warning
> > > without the suffix printed is clear as well. Having this slightly simpler
> > > diagnostic would simplify the code significantly.
> > *location*. which will still be a location, if only the first line of the
> > warning is displayed.
> >
> > We won't even win all that much.
> > Sure, we will return `Optional`, but we will still need to do
> > all that internal stuff to find the old suffix, uppercase it, compare them,
> > etc.
> >
> > And we lose a very useful ability to check what it considered to be the old
> > suffix in the tests.
> >
> > It is basically the same question as in D51949.
> > If you insist, sure, i can do that, but i *really* do believe this is
> > **WRONG**.
> And one more problem here, where do we point if we don't have a fix-it to get
> the suffix location from?
That point is valid, but if we dont have a suffix location, there is no suffix?
I dont insist :)
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:117
+
+ // So was this literal fully spelled,
+ // or is it a product of macro expansion and/or concatenation?
the comma is not necessary
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:119
+ // or is it a product of macro expansion and/or concatenation?
+ const bool RangeCanBeFixed =
+ utils::rangeCanBeFixed(ReplacementDsc.LiteralLocation, );
values are usually not annotated with `const`, please remove it for consistency
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:122
+
+ // Now, this is troubling. The literal may be somehow expanded from macro.
+ // So we can't just use plain getSourceRange(). An assumption is made that
one
please remove the `Now this is troubling`.
Please use `two or more (what, macros?)` instead of `two+`
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:192
+bool UppercaseLiteralSuffixCheck::checkBoundMatch(
+const ast_matchers::MatchFinder::MatchResult ) {
+ const auto *Literal =
`ast_matchers::` is not necessary, because there is a `using ...` at the top of
the file
Comment at: clang-tidy/utils/ASTUtils.cpp:72
+bool rangeIsEntirelyWithinMacroArgument(SourceRange Range,
+const SourceManager *SM) {
+ // Check if the range is entirely contained within a macro argument.
Does it make sense to have a `nullptr` for SM? I think you can use a reference
here
Comment at: docs/clang-tidy/checks/list.rst:67
bugprone-virtual-near-miss
cert-dcl03-c (redirects to misc-static-assert)
cert-dcl21-cpp
hicpp alias is missing here
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)
lebedev.ri updated this revision to Diff 167658.
lebedev.ri marked 16 inline comments as done.
lebedev.ri added a comment.
Herald added subscribers: Sanitizers, llvm-commits.
Addressed remaining review notes:
- Fixed dangling links in docs
- Don't mishandle user-defined literals
- Don't ignore macros, do check them. Thanks to @AaronBallman for the macro
concatenation example :)
- **Partial** fix-it support for macros.
...
Repository:
rCRT Compiler Runtime
https://reviews.llvm.org/D52670
Files:
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/cert/CMakeLists.txt
clang-tidy/hicpp/HICPPTidyModule.cpp
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/IdentifierNamingCheck.cpp
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
clang-tidy/readability/UppercaseLiteralSuffixCheck.h
clang-tidy/utils/ASTUtils.cpp
clang-tidy/utils/ASTUtils.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cert-dcl16-c.rst
docs/clang-tidy/checks/hicpp-uppercase-literal-suffix.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst
test/clang-tidy/readability-uppercase-literal-suffix-floating-point.cpp
test/clang-tidy/readability-uppercase-literal-suffix-hexadecimal-floating-point.cpp
test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp
test/clang-tidy/readability-uppercase-literal-suffix-integer.cpp
test/clang-tidy/readability-uppercase-literal-suffix.h
Index: test/clang-tidy/readability-uppercase-literal-suffix.h
===
--- /dev/null
+++ test/clang-tidy/readability-uppercase-literal-suffix.h
@@ -0,0 +1,16 @@
+template
+struct integral_constant {
+ static constexpr T value = v;
+ typedef T value_type;
+ typedef integral_constant type; // using injected-class-name
+ constexpr operator value_type() const noexcept { return value; }
+};
+
+using false_type = integral_constant;
+using true_type = integral_constant;
+
+template
+struct is_same : false_type {};
+
+template
+struct is_same : true_type {};
Index: test/clang-tidy/readability-uppercase-literal-suffix-integer.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-uppercase-literal-suffix-integer.cpp
@@ -0,0 +1,213 @@
+// RUN: %check_clang_tidy %s readability-uppercase-literal-suffix %t -- -- -I %S
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: clang-tidy %t.cpp -checks='-*,readability-uppercase-literal-suffix' -fix -- -I %S
+// RUN: clang-tidy %t.cpp -checks='-*,readability-uppercase-literal-suffix' -warnings-as-errors='-*,readability-uppercase-literal-suffix' -- -I %S
+
+#include "readability-uppercase-literal-suffix.h"
+
+void integer_suffix() {
+ static constexpr auto v0 = __LINE__; // synthetic
+ static_assert(v0 == 9 || v0 == 5, "");
+
+ static constexpr auto v1 = __cplusplus; // synthetic, long
+
+ static constexpr auto v2 = 1; // no literal
+ static_assert(is_same::value, "");
+ static_assert(v2 == 1, "");
+
+ // Unsigned
+
+ static constexpr auto v3 = 1u;
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: integer literal has suffix 'u', which is not upper-case
+ // CHECK-MESSAGES-NEXT: static constexpr auto v3 = 1u;
+ // CHECK-MESSAGES-NEXT: ^~
+ // CHECK-MESSAGES-NEXT: {{^ *}}U{{$}}
+ // CHECK-FIXES: static constexpr auto v3 = 1U;
+ static_assert(is_same::value, "");
+ static_assert(v3 == 1, "");
+
+ static constexpr auto v4 = 1U; // OK.
+ static_assert(is_same::value, "");
+ static_assert(v4 == 1, "");
+
+ // Long
+
+ static constexpr auto v5 = 1l;
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: integer literal has suffix 'l', which is not upper-case
+ // CHECK-MESSAGES-NEXT: static constexpr auto v5 = 1l;
+ // CHECK-MESSAGES-NEXT: ^~
+ // CHECK-MESSAGES-NEXT: {{^ *}}L{{$}}
+ // CHECK-FIXES: static constexpr auto v5 = 1L;
+ static_assert(is_same::value, "");
+ static_assert(v5 == 1, "");
+
+ static constexpr auto v6 = 1L; // OK.
+ static_assert(is_same::value, "");
+ static_assert(v6 == 1, "");
+
+ // Long Long
+
+ static constexpr auto v7 = 1ll;
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: integer literal has suffix 'll', which is not upper-case
+ // CHECK-MESSAGES-NEXT: static constexpr auto v7 = 1ll;
+ // CHECK-MESSAGES-NEXT: ^~~
+ // CHECK-MESSAGES-NEXT: {{^ *}}LL{{$}}
+ // CHECK-FIXES: static constexpr auto v7 = 1LL;
+ static_assert(is_same::value, "");
+ static_assert(v7 == 1, "");
+
+ static constexpr auto v8 = 1LL; // OK.
+ static_assert(is_same::value, "");
+ static_assert(v8 == 1, "");
+
+ // Unsigned Long
+
+ static constexpr auto v9 = 1ul;
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: integer literal has suffix 'ul', which is not upper-case
+ // CHECK-MESSAGES-NEXT: static constexpr auto v9 = 1ul;
+ // CHECK-MESSAGES-NEXT: ^~~
+ // CHECK-MESSAGES-NEXT: {{^ *}}UL{{$}}
+ // CHECK-FIXES: static constexpr auto
[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)
lebedev.ri added inline comments. Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:53 + const auto *T = dyn_cast(Node.getType().getTypePtr()); + if (!T) +return false; JonasToth wrote: > lebedev.ri wrote: > > JonasToth wrote: > > > Maybe the if could init `T`? It would require a second `return false;` if > > > i am not mistaken, but looks more regular to me. No strong opinion from > > > my side. > > Then we will not have an early-return, which is worse than this. > Ok. Can the `dyn_cast` be null at all? Shouldn't integerliteral always be a > `BuiltinType`? I don't know? I would guess no. Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:85 +template +llvm::Optional +UppercaseLiteralSuffixCheck::shouldReplaceLiteralSuffix( JonasToth wrote: > lebedev.ri wrote: > > JonasToth wrote: > > > These types get really long. Is it possible to put `NewSuffix` into the > > > anonymous namespace as well? > > No, because `shouldReplaceLiteralSuffix()` is a member function which > > returns that type. > > I think it should stay a member function, so theoretically `NewSuffix` > > could be a [second] template param, but that is kinda ugly.. > > I also can't simplify it via `using` because the `NewSuffix` is private. > > > > Perhaps we should keep this as-is? > Why does it need to be a member? It looks like it only accesses `NewSuffix` > which does nothing that requires private access to the check class. Passing `LangOpts` turned out to be sufficient to move it into anon namespace. Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165 + + diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case") + << LiteralType::Name << S.OldSuffix lebedev.ri wrote: > JonasToth wrote: > > lebedev.ri wrote: > > > JonasToth wrote: > > > > JonasToth wrote: > > > > > Lets move this `diag` into the true branch of the if stmt and drop > > > > > the ´else`. > > > > I think the warning should point at the suffix, which can be retrieved > > > > from the replacement range. Then you don't need to include the suffix > > > > itself in the diagnostic > > > If the warnings are aggregated (i.e. not raw `make` dump), with only the > > > first line shown, the suffix will still be invisible. > > > > > > Regarding the pointer direction, i'm not sure. > > > For now i have reworded the diag to justify pointing at the literal > > > itself. > > I don't understand that. The warning message does include the source > > location that would be clearly on the literal suffix and the warning > > without the suffix printed is clear as well. Having this slightly simpler > > diagnostic would simplify the code significantly. > *location*. which will still be a location, if only the first line of the > warning is displayed. > > We won't even win all that much. > Sure, we will return `Optional`, but we will still need to do all > that internal stuff to find the old suffix, uppercase it, compare them, etc. > > And we lose a very useful ability to check what it considered to be the old > suffix in the tests. > > It is basically the same question as in D51949. > If you insist, sure, i can do that, but i *really* do believe this is > **WRONG**. And one more problem here, where do we point if we don't have a fix-it to get the suffix location from? Repository: rCRT Compiler Runtime https://reviews.llvm.org/D52670 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.
EricWF added inline comments.
Comment at: docs/LanguageExtensions.rst:2179
+Clang provides experimental builtins to support C++ standard library
implementation
+of `std::experimental::source_location` as specified in
http://wg21.link/N4600.
+With the exception of `__builtin_COLUMN`, these builtins are also implemented
by
rsmith wrote:
> rST uses double-backticks for code font. Does this URL get autolinked?
Woops. Fixed.
Comment at: lib/AST/APValue.cpp:46-49
+template static bool isEmptyOrTombstoneKey(T const ) {
+ using DMI = llvm::DenseMapInfo;
+ return V == DMI::getEmptyKey() || V == DMI::getTombstoneKey();
+}
rsmith wrote:
> This seems to be unused?
It was used, but I folded the definition into that usage.
Comment at: lib/AST/ExprConstant.cpp:3353
+ APValue Str(LVal.Base, CharUnits::Zero(), APValue::NoLValuePath(), 0);
+ CompleteObject StrObj(, LVal.Base.getGlobalStringType()->desugar(),
+false);
rsmith wrote:
> Similarly, why do you need to desugar the type here?
It does seem redundant. Removing.
I think I was using it to generate a QualType from a Type*.
Comment at: lib/CodeGen/CGExprConstant.cpp:867-869
- llvm::Constant *VisitCXXDefaultArgExpr(CXXDefaultArgExpr *DAE, QualType T) {
-return Visit(DAE->getExpr(), T);
- }
rsmith wrote:
> Why was this removed?
I believe it was dead code?
Comment at: lib/CodeGen/CGExprConstant.cpp:868-869
llvm::Constant *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *DIE, QualType T)
{
+// FIXME: Is it ever possible for a SourceLocExpr to be below this node?
+// If so, we need to update the CurSourceLocExprScope in CodeGenFunction.
+
rsmith wrote:
> I think the salient point here is: this visitor will fail if it reaches a
> `SourceLocExpr`, so there's no need to track state that only it needs. (Same
> reason we didn't need a `CXXDefaultInitExprScope` for handling `CXXThisExpr`.)
Ack. Removing the comment.
https://reviews.llvm.org/D37035
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r343420 - Fix linkage error on ProgramPoint's dump method.
Author: ericwf
Date: Sun Sep 30 11:05:39 2018
New Revision: 343420
URL: http://llvm.org/viewvc/llvm-project?rev=343420=rev
Log:
Fix linkage error on ProgramPoint's dump method.
Currently, ProgramPoint::dump calls the out-of-line function
ProgramPoint::print. This causes
libraries which include ProgramPoint.h to become dependent on libclangAnalysis,
which in turn
causes missing symbol link error when building with -DBUILD_SHARED_LIBS=ON
-DLLVM_ENABLE_MODULES=ON.
The breakage was introduced in r343160.
This patch fixes the issues by moving ProgramPoint::dump's declaration out of
line.
Modified:
cfe/trunk/include/clang/Analysis/ProgramPoint.h
cfe/trunk/lib/Analysis/ProgramPoint.cpp
Modified: cfe/trunk/include/clang/Analysis/ProgramPoint.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/ProgramPoint.h?rev=343420=343419=343420=diff
==
--- cfe/trunk/include/clang/Analysis/ProgramPoint.h (original)
+++ cfe/trunk/include/clang/Analysis/ProgramPoint.h Sun Sep 30 11:05:39 2018
@@ -217,9 +217,7 @@ public:
void print(StringRef CR, llvm::raw_ostream ) const;
- LLVM_DUMP_METHOD void dump() const {
-return print(/*CR=*/"\n", llvm::errs());
- }
+ LLVM_DUMP_METHOD void dump() const;
static ProgramPoint getProgramPoint(const Stmt *S, ProgramPoint::Kind K,
const LocationContext *LC,
Modified: cfe/trunk/lib/Analysis/ProgramPoint.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ProgramPoint.cpp?rev=343420=343419=343420=diff
==
--- cfe/trunk/lib/Analysis/ProgramPoint.cpp (original)
+++ cfe/trunk/lib/Analysis/ProgramPoint.cpp Sun Sep 30 11:05:39 2018
@@ -43,6 +43,10 @@ ProgramPoint ProgramPoint::getProgramPoi
}
}
+LLVM_DUMP_METHOD void ProgramPoint::dump() const {
+ return print(/*CR=*/"\n", llvm::errs());
+}
+
static void printLocation(raw_ostream , SourceLocation SLoc,
const SourceManager ,
StringRef CR,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52696: Update ifunc attribute support documentation
joerg added a comment. I think this is still too optimistic. Full support for ifunc seems to be generally limited to x86. Most other architectures lack even definitions for anonymous ifunc relocations or support proper relaxation only in limited forms. That's especially annoying when looking at static linking. Repository: rL LLVM https://reviews.llvm.org/D52696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52334: [clang-tidy] Build it even without static analyzer
aaron.ballman reopened this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D52334#1250439, @aaron.ballman wrote: > I've commit as r343415, thank you for the patch! I've reverted in r343418 as the commit broke some bots. http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/37336 http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/24630 I also see that you have your own commit access, so I'll let you handle fixing up the failing tests and recommit. If there are not substantial changes required to fix the issue (e.g., it's just a matter of fixing up some tests), you're fine to fix them and commit without further review. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52334 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r343418 - Reverting r343415 as it breaks at least one of the bots.
Author: aaronballman
Date: Sun Sep 30 10:39:39 2018
New Revision: 343418
URL: http://llvm.org/viewvc/llvm-project?rev=343418=rev
Log:
Reverting r343415 as it breaks at least one of the bots.
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/37336
Modified:
clang-tools-extra/trunk/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
clang-tools-extra/trunk/clang-tidy/plugin/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp
clang-tools-extra/trunk/clang-tidy/tool/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
clang-tools-extra/trunk/docs/clang-tidy/index.rst
clang-tools-extra/trunk/test/CMakeLists.txt
clang-tools-extra/trunk/test/clang-tidy/enable-alpha-checks.cpp
clang-tools-extra/trunk/test/clang-tidy/mpi-buffer-deref.cpp
clang-tools-extra/trunk/test/clang-tidy/mpi-type-mismatch.cpp
clang-tools-extra/trunk/test/clang-tidy/nolint.cpp
clang-tools-extra/trunk/test/clang-tidy/read_file_config.cpp
clang-tools-extra/trunk/test/clang-tidy/static-analyzer-config.cpp
clang-tools-extra/trunk/test/clang-tidy/static-analyzer.cpp
clang-tools-extra/trunk/test/clang-tidy/temporaries.cpp
clang-tools-extra/trunk/test/lit.cfg
clang-tools-extra/trunk/unittests/CMakeLists.txt
Modified: clang-tools-extra/trunk/CMakeLists.txt
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/CMakeLists.txt?rev=343418=343417=343418=diff
==
--- clang-tools-extra/trunk/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/CMakeLists.txt Sun Sep 30 10:39:39 2018
@@ -1,8 +1,10 @@
add_subdirectory(clang-apply-replacements)
add_subdirectory(clang-reorder-fields)
add_subdirectory(modularize)
+if(CLANG_ENABLE_STATIC_ANALYZER)
add_subdirectory(clang-tidy)
add_subdirectory(clang-tidy-vs)
+endif()
add_subdirectory(change-namespace)
add_subdirectory(clang-doc)
Modified: clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/CMakeLists.txt?rev=343418=343417=343418=diff
==
--- clang-tools-extra/trunk/clang-tidy/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/CMakeLists.txt Sun Sep 30 10:39:39 2018
@@ -21,17 +21,12 @@ add_clang_library(clangTidy
clangLex
clangRewrite
clangSema
+ clangStaticAnalyzerCore
+ clangStaticAnalyzerFrontend
clangTooling
clangToolingCore
)
-if(CLANG_ENABLE_STATIC_ANALYZER)
- target_link_libraries(clangTidy PRIVATE
-clangStaticAnalyzerCore
-clangStaticAnalyzerFrontend
- )
-endif()
-
add_subdirectory(android)
add_subdirectory(abseil)
add_subdirectory(boost)
@@ -44,9 +39,7 @@ add_subdirectory(hicpp)
add_subdirectory(llvm)
add_subdirectory(misc)
add_subdirectory(modernize)
-if(CLANG_ENABLE_STATIC_ANALYZER)
- add_subdirectory(mpi)
-endif()
+add_subdirectory(mpi)
add_subdirectory(objc)
add_subdirectory(performance)
add_subdirectory(plugin)
Modified: clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp?rev=343418=343417=343418=diff
==
--- clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp Sun Sep 30 10:39:39 2018
@@ -34,10 +34,8 @@
#include "clang/Lex/Preprocessor.h"
#include "clang/Rewrite/Frontend/FixItRewriter.h"
#include "clang/Rewrite/Frontend/FrontendActions.h"
-#if CLANG_ENABLE_STATIC_ANALYZER
#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
-#endif // CLANG_ENABLE_STATIC_ANALYZER
#include "clang/Tooling/DiagnosticsYaml.h"
#include "clang/Tooling/Refactoring.h"
#include "clang/Tooling/ReplacementsYaml.h"
@@ -58,7 +56,6 @@ namespace clang {
namespace tidy {
namespace {
-#if CLANG_ENABLE_STATIC_ANALYZER
static const char *AnalyzerCheckNamePrefix = "clang-analyzer-";
class AnalyzerDiagnosticConsumer : public ento::PathDiagnosticConsumer {
@@ -90,7 +87,6 @@ public:
private:
ClangTidyContext
};
-#endif // CLANG_ENABLE_STATIC_ANALYZER
class ErrorReporter {
public:
@@ -300,7 +296,6 @@ ClangTidyASTConsumerFactory::ClangTidyAS
}
}
-#if CLANG_ENABLE_STATIC_ANALYZER
static void setStaticAnalyzerCheckerOpts(const ClangTidyOptions ,
AnalyzerOptionsRef AnalyzerOptions) {
StringRef AnalyzerPrefix(AnalyzerCheckNamePrefix);
@@ -344,7 +339,6 @@ static CheckersList getCheckersControlLi
}
return List;
}
-#endif // CLANG_ENABLE_STATIC_ANALYZER
std::unique_ptr
ClangTidyASTConsumerFactory::CreateASTConsumer(
@@ -386,7 +380,6 @@
[PATCH] D52334: [clang-tidy] Build it even without static analyzer
aaron.ballman closed this revision. aaron.ballman added a comment. I've commit as r343415, thank you for the patch! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52334 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r343415 - Allow clang-tidy to be built without a dependency on the clang static analyzer.
Author: aaronballman
Date: Sun Sep 30 10:22:58 2018
New Revision: 343415
URL: http://llvm.org/viewvc/llvm-project?rev=343415=rev
Log:
Allow clang-tidy to be built without a dependency on the clang static analyzer.
Patch by Stephen Kelly.
Modified:
clang-tools-extra/trunk/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
clang-tools-extra/trunk/clang-tidy/plugin/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp
clang-tools-extra/trunk/clang-tidy/tool/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
clang-tools-extra/trunk/docs/clang-tidy/index.rst
clang-tools-extra/trunk/test/CMakeLists.txt
clang-tools-extra/trunk/test/clang-tidy/enable-alpha-checks.cpp
clang-tools-extra/trunk/test/clang-tidy/mpi-buffer-deref.cpp
clang-tools-extra/trunk/test/clang-tidy/mpi-type-mismatch.cpp
clang-tools-extra/trunk/test/clang-tidy/nolint.cpp
clang-tools-extra/trunk/test/clang-tidy/read_file_config.cpp
clang-tools-extra/trunk/test/clang-tidy/static-analyzer-config.cpp
clang-tools-extra/trunk/test/clang-tidy/static-analyzer.cpp
clang-tools-extra/trunk/test/clang-tidy/temporaries.cpp
clang-tools-extra/trunk/test/lit.cfg
clang-tools-extra/trunk/unittests/CMakeLists.txt
Modified: clang-tools-extra/trunk/CMakeLists.txt
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/CMakeLists.txt?rev=343415=343414=343415=diff
==
--- clang-tools-extra/trunk/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/CMakeLists.txt Sun Sep 30 10:22:58 2018
@@ -1,10 +1,8 @@
add_subdirectory(clang-apply-replacements)
add_subdirectory(clang-reorder-fields)
add_subdirectory(modularize)
-if(CLANG_ENABLE_STATIC_ANALYZER)
add_subdirectory(clang-tidy)
add_subdirectory(clang-tidy-vs)
-endif()
add_subdirectory(change-namespace)
add_subdirectory(clang-doc)
Modified: clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/CMakeLists.txt?rev=343415=343414=343415=diff
==
--- clang-tools-extra/trunk/clang-tidy/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/CMakeLists.txt Sun Sep 30 10:22:58 2018
@@ -21,12 +21,17 @@ add_clang_library(clangTidy
clangLex
clangRewrite
clangSema
- clangStaticAnalyzerCore
- clangStaticAnalyzerFrontend
clangTooling
clangToolingCore
)
+if(CLANG_ENABLE_STATIC_ANALYZER)
+ target_link_libraries(clangTidy PRIVATE
+clangStaticAnalyzerCore
+clangStaticAnalyzerFrontend
+ )
+endif()
+
add_subdirectory(android)
add_subdirectory(abseil)
add_subdirectory(boost)
@@ -39,7 +44,9 @@ add_subdirectory(hicpp)
add_subdirectory(llvm)
add_subdirectory(misc)
add_subdirectory(modernize)
-add_subdirectory(mpi)
+if(CLANG_ENABLE_STATIC_ANALYZER)
+ add_subdirectory(mpi)
+endif()
add_subdirectory(objc)
add_subdirectory(performance)
add_subdirectory(plugin)
Modified: clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp?rev=343415=343414=343415=diff
==
--- clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp Sun Sep 30 10:22:58 2018
@@ -34,8 +34,10 @@
#include "clang/Lex/Preprocessor.h"
#include "clang/Rewrite/Frontend/FixItRewriter.h"
#include "clang/Rewrite/Frontend/FrontendActions.h"
+#if CLANG_ENABLE_STATIC_ANALYZER
#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#endif // CLANG_ENABLE_STATIC_ANALYZER
#include "clang/Tooling/DiagnosticsYaml.h"
#include "clang/Tooling/Refactoring.h"
#include "clang/Tooling/ReplacementsYaml.h"
@@ -56,6 +58,7 @@ namespace clang {
namespace tidy {
namespace {
+#if CLANG_ENABLE_STATIC_ANALYZER
static const char *AnalyzerCheckNamePrefix = "clang-analyzer-";
class AnalyzerDiagnosticConsumer : public ento::PathDiagnosticConsumer {
@@ -87,6 +90,7 @@ public:
private:
ClangTidyContext
};
+#endif // CLANG_ENABLE_STATIC_ANALYZER
class ErrorReporter {
public:
@@ -296,6 +300,7 @@ ClangTidyASTConsumerFactory::ClangTidyAS
}
}
+#if CLANG_ENABLE_STATIC_ANALYZER
static void setStaticAnalyzerCheckerOpts(const ClangTidyOptions ,
AnalyzerOptionsRef AnalyzerOptions) {
StringRef AnalyzerPrefix(AnalyzerCheckNamePrefix);
@@ -339,6 +344,7 @@ static CheckersList getCheckersControlLi
}
return List;
}
+#endif // CLANG_ENABLE_STATIC_ANALYZER
std::unique_ptr
ClangTidyASTConsumerFactory::CreateASTConsumer(
@@ -380,6 +386,7 @@ ClangTidyASTConsumerFactory::CreateASTCo
if
Re: r342827 - Fix modules build with shared library.
+rsmith (actually this time)
On Sun, Sep 30, 2018 at 12:09 PM Eric Fiselier wrote:
> +rsmith
>
> Hi All,
>
> Sorry, I'm not actually sure why this fix is correct.I stole both the fix
> and the comment from a similar one on L150 of the module map left by
> Richard Smith.
>
> /Eric
>
> On Tue, Sep 25, 2018 at 8:36 PM Shuai Wang wrote:
>
>> I'd like to understand this better as well, in particular what would be a
>> proper fix?
>>
>> On Tue, Sep 25, 2018 at 2:15 PM David Blaikie wrote:
>>
>>> +Shuai Wang
>>>
>>> On Tue, Sep 25, 2018 at 2:14 PM David Blaikie
>>> wrote:
>>>
Hey Eric - thanks for the fix - but could you explain the issue here in
a bit more detail, as I'm a bit confused (& really interested in
understanding any layering problems in LLVM - and fixing them/making sure
they're fixed/holding the line/etc)
What do you mean by "pull all of the AST matchers library into clang" -
how does including a header ever add a link dependency?
- Dave
On Sat, Sep 22, 2018 at 5:49 PM Eric Fiselier via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> Author: ericwf
> Date: Sat Sep 22 17:48:05 2018
> New Revision: 342827
>
> URL: http://llvm.org/viewvc/llvm-project?rev=342827=rev
> Log:
> Fix modules build with shared library.
>
> r341994 caused clangAnalysis to pull all of the AST matchers
> library into clang. Due to inline key functions in the headers,
> importing the AST matchers library gives a link dependency on the
> AST matchers (and thus the AST), which clang should not
> have.
>
> This patch works around the issues by excluding the offending
> libclangAnalysis header in the modulemap.
>
> Modified:
> cfe/trunk/include/clang/module.modulemap
>
> Modified: cfe/trunk/include/clang/module.modulemap
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/module.modulemap?rev=342827=342826=342827=diff
>
> ==
> --- cfe/trunk/include/clang/module.modulemap (original)
> +++ cfe/trunk/include/clang/module.modulemap Sat Sep 22 17:48:05 2018
> @@ -5,6 +5,12 @@ module Clang_Analysis {
>textual header "Analysis/Analyses/ThreadSafetyOps.def"
>
>module * { export * }
> +
> + // FIXME: Exclude these headers to avoid pulling all of the AST
> matchers
> + // library into clang. Due to inline key functions in the headers,
> + // importing the AST matchers library gives a link dependency on
> the AST
> + // matchers (and thus the AST), which clang-format should not have.
> + exclude header "Analysis/Analyses/ExprMutationAnalyzer.h"
> }
>
> module Clang_AST {
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r342827 - Fix modules build with shared library.
+rsmith
Hi All,
Sorry, I'm not actually sure why this fix is correct.I stole both the fix
and the comment from a similar one on L150 of the module map left by
Richard Smith.
/Eric
On Tue, Sep 25, 2018 at 8:36 PM Shuai Wang wrote:
> I'd like to understand this better as well, in particular what would be a
> proper fix?
>
> On Tue, Sep 25, 2018 at 2:15 PM David Blaikie wrote:
>
>> +Shuai Wang
>>
>> On Tue, Sep 25, 2018 at 2:14 PM David Blaikie wrote:
>>
>>> Hey Eric - thanks for the fix - but could you explain the issue here in
>>> a bit more detail, as I'm a bit confused (& really interested in
>>> understanding any layering problems in LLVM - and fixing them/making sure
>>> they're fixed/holding the line/etc)
>>>
>>> What do you mean by "pull all of the AST matchers library into clang" -
>>> how does including a header ever add a link dependency?
>>>
>>> - Dave
>>>
>>>
>>> On Sat, Sep 22, 2018 at 5:49 PM Eric Fiselier via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
Author: ericwf
Date: Sat Sep 22 17:48:05 2018
New Revision: 342827
URL: http://llvm.org/viewvc/llvm-project?rev=342827=rev
Log:
Fix modules build with shared library.
r341994 caused clangAnalysis to pull all of the AST matchers
library into clang. Due to inline key functions in the headers,
importing the AST matchers library gives a link dependency on the
AST matchers (and thus the AST), which clang should not
have.
This patch works around the issues by excluding the offending
libclangAnalysis header in the modulemap.
Modified:
cfe/trunk/include/clang/module.modulemap
Modified: cfe/trunk/include/clang/module.modulemap
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/module.modulemap?rev=342827=342826=342827=diff
==
--- cfe/trunk/include/clang/module.modulemap (original)
+++ cfe/trunk/include/clang/module.modulemap Sat Sep 22 17:48:05 2018
@@ -5,6 +5,12 @@ module Clang_Analysis {
textual header "Analysis/Analyses/ThreadSafetyOps.def"
module * { export * }
+
+ // FIXME: Exclude these headers to avoid pulling all of the AST
matchers
+ // library into clang. Due to inline key functions in the headers,
+ // importing the AST matchers library gives a link dependency on the
AST
+ // matchers (and thus the AST), which clang-format should not have.
+ exclude header "Analysis/Analyses/ExprMutationAnalyzer.h"
}
module Clang_AST {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52703: Allow ifunc resolvers to accept arguments
emaste created this revision.
emaste added a reviewer: DmitryPolukhin.
Herald added a reviewer: javed.absar.
Herald added subscribers: kristof.beyls, krytarowski.
When ifunc support was added to Clang (in https://reviews.llvm.org/rC265917) it
did not allow resolvers to take function arguments. This was based on GCC's
documentation, which states resolvers return a pointer and take no arguments.
However, GCC actually allows it, and glibc (on non-x86 platforms) and FreeBSD
(on x86 and arm64) pass CPU identification information as arguments to ifunc
resolvers. I believe GCC's documentation is simply incorrect / out-of-date.
We've removed the prohibition in FreeBSD's in-tree Clang:
https://svnweb.freebsd.org/changeset/base/339019.
https://reviews.llvm.org/D52703
Files:
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/CodeGen/CodeGenModule.cpp
test/Sema/attr-ifunc.c
Index: test/Sema/attr-ifunc.c
===
--- test/Sema/attr-ifunc.c
+++ test/Sema/attr-ifunc.c
@@ -27,10 +27,6 @@
void f4() __attribute__((ifunc("f4_ifunc")));
//expected-error@-1 {{ifunc resolver function must return a pointer}}
-void* f5_ifunc(int i) { return 0; }
-void f5() __attribute__((ifunc("f5_ifunc")));
-//expected-error@-1 {{ifunc resolver function must have no parameters}}
-
#else
void f1a() __asm("f1");
void f1a() {}
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -322,8 +322,6 @@
assert(FTy);
if (!FTy->getReturnType()->isPointerTy())
Diags.Report(Location, diag::err_ifunc_resolver_return);
- if (FTy->getNumParams())
-Diags.Report(Location, diag::err_ifunc_resolver_params);
}
llvm::Constant *Aliasee = Alias->getIndirectSymbol();
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2887,8 +2887,6 @@
"%select{alias|ifunc}0 definition is part of a cycle">;
def err_ifunc_resolver_return : Error<
"ifunc resolver function must return a pointer">;
-def err_ifunc_resolver_params : Error<
- "ifunc resolver function must have no parameters">;
def warn_attribute_wrong_decl_type_str : Warning<
"%0 attribute only applies to %1">, InGroup;
def err_attribute_wrong_decl_type_str : Error<
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -3366,7 +3366,7 @@
let Content = [{
``__attribute__((ifunc("resolver")))`` is used to mark that the address of a
declaration should be resolved at runtime by calling a resolver function.
-The symbol name of the resolver function is given in quotes. A function with
this name (after mangling) must be defined in the current translation unit; it
may be ``static``. The resolver function should take no arguments and return a
pointer.
+The symbol name of the resolver function is given in quotes. A function with
this name (after mangling) must be defined in the current translation unit; it
may be ``static``. The resolver function should return a pointer.
The ``ifunc`` attribute may only be used on a function declaration. A
function declaration with an ``ifunc`` attribute is considered to be a
definition of the declared entity. The entity must not have weak linkage; for
example, in C++, it cannot be applied to a declaration if a definition at that
location would be considered inline.
Index: test/Sema/attr-ifunc.c
===
--- test/Sema/attr-ifunc.c
+++ test/Sema/attr-ifunc.c
@@ -27,10 +27,6 @@
void f4() __attribute__((ifunc("f4_ifunc")));
//expected-error@-1 {{ifunc resolver function must return a pointer}}
-void* f5_ifunc(int i) { return 0; }
-void f5() __attribute__((ifunc("f5_ifunc")));
-//expected-error@-1 {{ifunc resolver function must have no parameters}}
-
#else
void f1a() __asm("f1");
void f1a() {}
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -322,8 +322,6 @@
assert(FTy);
if (!FTy->getReturnType()->isPointerTy())
Diags.Report(Location, diag::err_ifunc_resolver_return);
- if (FTy->getNumParams())
-Diags.Report(Location, diag::err_ifunc_resolver_params);
}
llvm::Constant *Aliasee = Alias->getIndirectSymbol();
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2887,8 +2887,6 @@
"%select{alias|ifunc}0
[PATCH] D52696: Update ifunc attribute support documentation
This revision was automatically updated to reflect the committed changes. Closed by commit rL343408: Update ifunc attribute support documentation (authored by emaste, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D52696?vs=167626=167645#toc Repository: rL LLVM https://reviews.llvm.org/D52696 Files: cfe/trunk/include/clang/Basic/AttrDocs.td Index: cfe/trunk/include/clang/Basic/AttrDocs.td === --- cfe/trunk/include/clang/Basic/AttrDocs.td +++ cfe/trunk/include/clang/Basic/AttrDocs.td @@ -3370,7 +3370,7 @@ The ``ifunc`` attribute may only be used on a function declaration. A function declaration with an ``ifunc`` attribute is considered to be a definition of the declared entity. The entity must not have weak linkage; for example, in C++, it cannot be applied to a declaration if a definition at that location would be considered inline. -Not all targets support this attribute. ELF targets support this attribute when using binutils v2.20.1 or higher and glibc v2.11.1 or higher. Non-ELF targets currently do not support this attribute. +Not all targets support this attribute. ELF target support depends on both the linker and runtime linker, and is available in at least lld 4.0 and later, binutils 2.20.1 and later, glibc v2.11.1 and later, and FreeBSD 9.1 and later. Non-ELF targets currently do not support this attribute. }]; } Index: cfe/trunk/include/clang/Basic/AttrDocs.td === --- cfe/trunk/include/clang/Basic/AttrDocs.td +++ cfe/trunk/include/clang/Basic/AttrDocs.td @@ -3370,7 +3370,7 @@ The ``ifunc`` attribute may only be used on a function declaration. A function declaration with an ``ifunc`` attribute is considered to be a definition of the declared entity. The entity must not have weak linkage; for example, in C++, it cannot be applied to a declaration if a definition at that location would be considered inline. -Not all targets support this attribute. ELF targets support this attribute when using binutils v2.20.1 or higher and glibc v2.11.1 or higher. Non-ELF targets currently do not support this attribute. +Not all targets support this attribute. ELF target support depends on both the linker and runtime linker, and is available in at least lld 4.0 and later, binutils 2.20.1 and later, glibc v2.11.1 and later, and FreeBSD 9.1 and later. Non-ELF targets currently do not support this attribute. }]; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r343408 - Update ifunc attribute support documentation
Author: emaste Date: Sun Sep 30 08:08:18 2018 New Revision: 343408 URL: http://llvm.org/viewvc/llvm-project?rev=343408=rev Log: Update ifunc attribute support documentation Previously we documented GNU binutils and glibc versions required for ifunc support, but our own lld linker and FreeBSD's rtld also support ifuncs. Differential Revision: https://reviews.llvm.org/D52696 Modified: cfe/trunk/include/clang/Basic/AttrDocs.td Modified: cfe/trunk/include/clang/Basic/AttrDocs.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=343408=343407=343408=diff == --- cfe/trunk/include/clang/Basic/AttrDocs.td (original) +++ cfe/trunk/include/clang/Basic/AttrDocs.td Sun Sep 30 08:08:18 2018 @@ -3370,7 +3370,7 @@ The symbol name of the resolver function The ``ifunc`` attribute may only be used on a function declaration. A function declaration with an ``ifunc`` attribute is considered to be a definition of the declared entity. The entity must not have weak linkage; for example, in C++, it cannot be applied to a declaration if a definition at that location would be considered inline. -Not all targets support this attribute. ELF targets support this attribute when using binutils v2.20.1 or higher and glibc v2.11.1 or higher. Non-ELF targets currently do not support this attribute. +Not all targets support this attribute. ELF target support depends on both the linker and runtime linker, and is available in at least lld 4.0 and later, binutils 2.20.1 and later, glibc v2.11.1 and later, and FreeBSD 9.1 and later. Non-ELF targets currently do not support this attribute. }]; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52696: Update ifunc attribute support documentation
DmitryPolukhin accepted this revision. DmitryPolukhin added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D52696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29660: [OpenMP] Add flag for overwriting default PTX version for OpenMP targets
Hahnfeld added a comment. Herald added subscribers: llvm-commits, guansong. Going through my list of reviews, this patch was reverted because of memory leaks in other changes. However, I don't think we need this anymore because Clang is raising the PTX level as needed for that CUDA version. Can we abandon this flag? Repository: rL LLVM https://reviews.llvm.org/D29660 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52438: [CUDA] Add basic support for CUDA-10.0
Hahnfeld added a comment. I think this revision can be closed after https://reviews.llvm.org/rC342924? https://reviews.llvm.org/D52438 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52437: [CUDA] Add preliminary support for CUDA 10.0
Hahnfeld removed a reviewer: Hahnfeld. Hahnfeld added a comment. I think this revision can be closed after https://reviews.llvm.org/rC342924? Repository: rC Clang https://reviews.llvm.org/D52437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50850: clang: Add triples support for MIPS r6
atanasyan added a comment. In https://reviews.llvm.org/D50850#1250285, @wzssyqa wrote: > This is really for Clang. I guess you mean that compiler-rt directory also > need to be patched. If you take a look at the previous version of this patch https://reviews.llvm.org/D50850?id=167419, you see that it changed LLVM files. Probably you attached another diff. As to `compiler-rt` - the Phabricator replace `R6` symbols by the `https://reviews.llvm.org/source/compiler-rt/` links. So my statement was "Could you attach an actual patch brings `R6` support to the Clang driver?". Comment at: lib/Driver/ToolChains/Gnu.cpp:2093 BiarchTripleAliases.append(begin(MIPSELTriples), end(MIPSELTriples)); -BiarchTripleAliases.append(begin(MIPSTriples), end(MIPSTriples)); +BiarchLibDirs.append(begin(MIPSN32ELLibDirs), end(MIPSN32ELLibDirs)); +BiarchTripleAliases.append(begin(MIPSN32ELTriples), end(MIPSN32ELTriples)); wzssyqa wrote: > atanasyan wrote: > > Ditto > As a question: why MIPSTriples here? > the above mips64 lines don't include any EL Triples. I do not remember exact answer, although I might be an author if this code. Maybe it handle some complicated directories tree from multi-arch toolchains. Are all tests passed if you remove this line? Comment at: lib/Driver/ToolChains/Gnu.cpp:2437 if (getTriple().getEnvironment() == llvm::Triple::GNUABI64 || -getTriple().isAndroid() || -getTriple().isOSFreeBSD() || +getTriple().getEnvironment() == llvm::Triple::GNUABIN32 || +getTriple().isAndroid() || getTriple().isOSFreeBSD() || wzssyqa wrote: > atanasyan wrote: > > Are you sure that integrated LLVM assembler is ready to support N32 code > > generation? Anyway this change is for a separate patch. > I didn't have so many test, while helloworld programs really works. > > You created a patch that teaches the Clang driver to understand (pass arguments to backend, find includes and libraries etc) N32 ABI better. Now I can compile "hello world". And after that you decided that LLVM backend does not have any MIPS N32 ABI related problems. I think we can enable the integrated assembler when a) it's possible to recursively build Clang with N32 ABI, b) all tests from LLVM test suite (https://llvm.org/docs/TestSuiteGuide.html) are passed in N32 ABI mode, c) we check that LLVM produces correct DWARF for N32. https://reviews.llvm.org/D50850 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51464: clang: fix MIPS/N32 triple and paths
atanasyan added a comment. Please run test suite before sending a patch to review. After applying this patch the following tests failed: - test/CodeGen/target-data.c - test/Driver/mips-cs.cpp https://reviews.llvm.org/D51464 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
