Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks generally good - few things might be able to be tidied up/simplified, I think?
| clang/unittests/AST/ASTPrint.h | ||
|---|---|---|
| 67–68 | Why is this implemented via specialization but other configuration is implemented via functors (like Printer and PolicyAdjuster) are done via passed in functors? Might be worth using the same mechanism for all of them - this one can have a default argument value in the ctor so it doesn't have to be specified by all uses of this class/they can get this default behavior as implemented here. | |
| clang/unittests/AST/DeclPrinterTest.cpp | ||
| 53 | Is the PrinterType<Decl>{} needed here, or could PrintDecl be passed directly/without explicitly constructing the wrapper? (functions should be implicitly convertible to the lambda type, I think?) Similarly for all the PrintingPolicyAdjuster(...) - might be able to skip that wrapping expression & rely on an implicit conversion. | |
| clang/unittests/AST/StmtPrinterTest.cpp | ||
| 55–56 | I guess maybe this could be changed (better or worse, not sure) to this: return PrintedNodeMatches<Stmt>(..., PrintStmt, ...); | |
Address review comments
| clang/unittests/AST/DeclPrinterTest.cpp | ||
|---|---|---|
| 53 | Good call on both counts. (The PrintingPolicyAdjuster(...) stuff was left over from when the typedef was for an Optional<function_ref> which you correctly observed was redundant.) | |
This change I suspect is causing a build failure on a build bot.
https://lab.llvm.org/buildbot/#/builders/110/builds/4827
FAILED: /usr/bin/c++ -DCLANG_ROUND_TRIP_CC1_ARGS=ON -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/unittests/AST -I/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/clang/unittests/AST -I/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/clang/include -Itools/clang/include -Iinclude -I/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/llvm/include -I/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/llvm/utils/unittest/googletest/include -I/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/llvm/utils/unittest/googlemock/include -march=broadwell -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 -DNDEBUG -Wno-variadic-macros -fno-exceptions -fno-rtti -UNDEBUG -Wno-suggest-override -std=c++14 -MD -MT tools/clang/unittests/AST/CMakeFiles/ASTTests.dir/StmtPrinterTest.cpp.o -MF tools/clang/unittests/AST/CMakeFiles/ASTTests.dir/StmtPrinterTest.cpp.o.d -o tools/clang/unittests/AST/CMakeFiles/ASTTests.dir/StmtPrinterTest.cpp.o -c /home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/clang/unittests/AST/StmtPrinterTest.cpp /tmp/cc8Ycpsn.s: Assembler messages: /tmp/cc8Ycpsn.s:11414: Error: symbol `_ZNSt17_Function_handlerIFbPKN5clang4StmtEENS0_UlS3_E2_EE9_M_invokeERKSt9_Any_dataOS3_' is already defined /tmp/cc8Ycpsn.s:11937: Error: symbol `_ZNSt14_Function_base13_Base_managerIN5clangUlPKNS1_4StmtEE2_EE10_M_managerERSt9_Any_dataRKS7_St18_Manager_operation' is already defined ninja: build stopped: subcommand failed.
Can you take a look?
Indeed, it also breaks the cross-build for M68k: http://lab.llvm.org:8014/#/builders/180/builds/341
Any ideas what version of the standard library these buildbots are using? This /looks/ a bit like a bug in the standard library in use, perhaps? (also because it's not showing up in lots of other buildbots/developers, I assume?)
To be honest, I don't really understand this error. It seems to come from a compile step (not a link step), and it comes from the assembler and says "symbol ... is already defined"? I don't think I've seen an error like that before. (Typically, errors about duplicate definitions would come from the linker.)
Any suggestions for what it might indicate or how to investigate are welcome.
I cannot speak for the other builds bots, but the cross-buildbot for m68k which I maintain is running openSUSE Leap 15.3 with GCC 7.5.0.
Maybe this change is compatible with newer versions of GCC only?
I'm not sure how to get the standard library version that is being used, but our internal upstream build bot hit this issue, and it is running ubuntu 18.04 with gcc 7.5 as well.
My main dev machine is running ubuntu 20.04 with 9.3 and that does not show the failure, but another one of our internal linux machines with 18.04 and gcc 7.5 does show the failure.
Yeah, I'm with you there.
@dyung @glaubitz - perhaps you two could help with a reduced example of what causes this, it does look like a bug in the compiler (yeah, I'd at first assumed a bug in the standard library that caused a link error, but as @nridge pointed out it's not a link error but an assembly error - which really looks like a compiler bug to me) - but likely there's some room for a workaround if we knew what we were dealing with and how to workaround it reliably.
If it helps, I have so far been able to reduce the file to this which still shows the failure when compiled with gcc 7.5:
#include "ASTPrint.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
#include "gtest/gtest.h"
using namespace clang;
using namespace ast_matchers;
//using namespace tooling;
namespace {
enum class StdVer { CXX98, CXX11, CXX14, CXX17, CXX2a };
DeclarationMatcher FunctionBodyMatcher(StringRef ContainingFunction) {
return functionDecl(hasName(ContainingFunction),
has(compoundStmt(has(stmt().bind("id")))));
}
static void PrintStmt(raw_ostream &Out, const ASTContext *Context,
const Stmt *S, PrintingPolicyAdjuster PolicyAdjuster) {
assert(S != nullptr && "Expected non-null Stmt");
PrintingPolicy Policy = Context->getPrintingPolicy();
if (PolicyAdjuster)
PolicyAdjuster(Policy);
S->printPretty(Out, /*Helper*/ nullptr, Policy);
}
template <typename Matcher>
::testing::AssertionResult
PrintedStmtMatches(StringRef Code, const std::vector<std::string> &Args,
const Matcher &NodeMatch, StringRef ExpectedPrinted,
PrintingPolicyAdjuster PolicyAdjuster = nullptr) {
return PrintedNodeMatches<Stmt>(Code, Args, NodeMatch, ExpectedPrinted, "",
PrintStmt, PolicyAdjuster);
}
template <typename T>
::testing::AssertionResult
PrintedStmtCXXMatches(StdVer Standard, StringRef Code, const T &NodeMatch,
StringRef ExpectedPrinted,
PrintingPolicyAdjuster PolicyAdjuster = nullptr) {
const char *StdOpt;
switch (Standard) {
case StdVer::CXX98: StdOpt = "-std=c++98"; break;
case StdVer::CXX11: StdOpt = "-std=c++11"; break;
case StdVer::CXX14: StdOpt = "-std=c++14"; break;
case StdVer::CXX17: StdOpt = "-std=c++17"; break;
case StdVer::CXX2a: StdOpt = "-std=c++2a"; break;
}
std::vector<std::string> Args = {
StdOpt,
"-Wno-unused-value",
};
return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted,
PolicyAdjuster);
}
} // unnamed namespace
TEST(StmtPrinter, TestIntegerLiteral) {
ASSERT_TRUE(PrintedStmtCXXMatches(StdVer::CXX98,
"void A() {"
" 1, -1, 1U, 1u,"
" 1L, 1l, -1L, 1UL, 1ul,"
" 1LL, -1LL, 1ULL;"
"}",
FunctionBodyMatcher("A"),
"1 , -1 , 1U , 1U , "
"1L , 1L , -1L , 1UL , 1UL , "
"1LL , -1LL , 1ULL"));
// Should be: with semicolon
}
TEST(StmtPrinter, TestCXXConversionDeclImplicit) {
ASSERT_TRUE(PrintedStmtCXXMatches(
StdVer::CXX98,
"struct A {"
"operator void *();"
"A operator&(A);"
"};"
"void bar(void *);"
"void foo(A a, A b) {"
" bar(a & b);"
"}",
traverse(TK_AsIs, cxxMemberCallExpr(anything()).bind("id")), "a & b"));
}When compiled with gcc 7.5, it produces the attached (gzip'd) assembly file which is what is what is giving the errors:
GNU assembler version 2.30 (x86_64-linux-gnu) using BFD version (GNU Binutils for Ubuntu) 2.30 StmtPrinterTest2.s: Assembler messages: StmtPrinterTest2.s:928: Error: symbol `_ZNSt14_Function_base13_Base_managerIN5clangUlPKNS1_4StmtEE2_EE10_M_managerERSt9_Any_dataRKS7_St18_Manager_operation' is already defined StmtPrinterTest2.s:9924: Error: symbol `_ZNSt17_Function_handlerIFbPKN5clang4StmtEENS0_UlS3_E2_EE9_M_invokeERKSt9_Any_dataOS3_' is already defined
Could you provide the preprocessed input file - maybe that run through creduce could be helpful.
I did originally run it through cvise, but wasn't sure if it would be useful since it cut out everything except for what was causing one of the two failures. But if it helps, here is what it reduced to:
namespace std {
template <bool> using __bool_constant = int;
template <typename> struct is_base_of;
template <typename> using remove_reference_t = int;
template <bool> using enable_if_t = int;
template <typename> struct iterator_traits;
struct _Any_data;
enum _Manager_operation {};
template <typename> class function;
class _Function_base {
public:
template <typename> class _Base_manager {
public:
static bool _M_manager(_Any_data &, const _Any_data &, _Manager_operation) {
}
};
typedef bool (*_Manager_type)(_Any_data &, const _Any_data &,
_Manager_operation);
_Manager_type _M_manager;
};
template <typename, typename> class _Function_handler;
template <typename _Res, typename _Functor, typename... _ArgTypes>
class _Function_handler<_Res(_ArgTypes...), _Functor>
: public _Function_base::_Base_manager<_Functor> {};
template <typename _Res, typename... _ArgTypes>
class function<_Res(_ArgTypes...)> : _Function_base {
template <typename, typename> using _Requires = int;
public:
template <typename _Functor,
typename = _Requires<__bool_constant<!bool()>, void>,
typename = _Requires<int, void>>
function(_Functor);
};
template <typename _Res, typename... _ArgTypes>
template <typename _Functor, typename, typename>
function<_Res(_ArgTypes...)>::function(_Functor) {
_M_manager = _Function_handler<_Res(), _Functor>::_M_manager;
}
} // namespace std
template <typename IterTy>
void hasNItems(
IterTy Begin = [] {},
std::enable_if_t<std::is_base_of<std::iterator_traits<
std::remove_reference_t<decltype(Begin)>>>::valuevoid> = [] {},
std::enable_if_t<std::is_base_of<std::iterator_traits<
std::remove_reference_t<decltype(Begin)>>>::valuevoid> = [] {});
class StringRef {
public:
StringRef(char *);
};
namespace clang {
class Stmt anything();
template <typename> using NodeFilter = std::function<bool()>;
template <typename NodeType, typename Matcher>
void PrintedNodeMatches(
Matcher, NodeFilter<NodeType> = [](const NodeType *) {});
} // namespace clang
using namespace clang;
enum StdVer { CXX98 };
template <typename Matcher> void PrintedStmtMatches(Matcher NodeMatch) {
PrintedNodeMatches<Stmt>(NodeMatch);
}
template <typename T>
void PrintedStmtCXXMatches(StdVer, StringRef, T NodeMatch, StringRef) {
PrintedStmtMatches(NodeMatch);
PrintedStmtCXXMatches(CXX98, "", 0, "");
}
void StmtPrinter_TestCXXConversionDeclImplicit_TestTestBody() {
PrintedStmtCXXMatches(CXX98, "", anything, "");
}Compiled with gcc 7.5 and optimizations yields the following output:
dyung@frogstar:~/sandbox/StmtPrinterTest$ gcc -O2 -Wno-write-strings StmtPrinterTest.preproc.cpp /tmp/ccLNKEkE.s: Assembler messages: /tmp/ccLNKEkE.s:14: Error: symbol `_ZNSt14_Function_base13_Base_managerIN5clangUlPKNS1_4StmtEE2_EE10_M_managerERSt9_Any_dataRKS7_St18_Manager_operation' is already defined
Thanks! I reduced it further to:
template <typename>
class function {
public:
template <typename F>
function(F) {}
};
template <typename M>
void Foo(M, function<void()> = [](){});
void Bar() {
Foo(42);
Foo(42.0);
}with gcc 7, this gives:
/tmp/cccKjL8O.s: Assembler messages: /tmp/cccKjL8O.s:76: Error: symbol `_ZN8functionIFvvEEC2IUlvE_EET_' is already defined
I still don't understand this error, but it seems to be related to the lambda that appears as a default argument in a function that gets instantiated multiple times (and with the default argument value not dependent on the function's template parameter), so I think that gives us some idea of where to try workarounds.
Making the default argument a non-lambda seems to be sufficient to avoid the error:
template <typename>
class function {
public:
template <typename F>
function(F) {}
};
void DefaultFunc();
template <typename M>
void Foo(M, function<void()> = DefaultFunc);
void Bar() {
Foo(42);
Foo(42.0);
}Interesting. If you can get me an updated patch, I can give it a try on my machine with 7.5 to verify if you like.
I can confirm that it builds and tests successfully on a machine with gcc 7.5. Assuming you have tested with more recent compilers and that it is fine there, LGTM.
Why is this implemented via specialization but other configuration is implemented via functors (like Printer and PolicyAdjuster) are done via passed in functors? Might be worth using the same mechanism for all of them - this one can have a default argument value in the ctor so it doesn't have to be specified by all uses of this class/they can get this default behavior as implemented here.