Bump to llvm-12, rebasing out-of-tree patches onto it #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pfez
force-pushed
the
feature/llvm-12
branch
2 times, most recently
from
8f79642 to
3b8a5ce
Compare
|
UPDATE
|
|
Looks great! LLVM 12, here we come. Just a couple of comments:
|
|
We have a failing test due to the fact that we're running the test suite as root. Basically, a command is expected to fail but it doesn't since root bypasses permissions checks. Given also the other problematic tests, I suggest we have a series of commits in our LLVM fork that disable tests that we know are going to fail but we don't really care that much. Then, every time we upgrade LLVM we drop those commits and see if any of these issue has been solved. I know this is suboptimal, but I think it's the most reasonable thing to do. These are the two tests that are failing due to permission issues: Can you please disable them? |
Done! |
Starting from C++20, the equality comparison should be reversible, i.e.
the expression `A == B` and `B == A` should select the same overload for
`operator==()`. If this does not happen, the language treats the call to
`operator==()` as ambiguous.
The ASSERT_EQ performs an equality comparison under the hood, using
`operator==()`. In the following expression, the LHS has type int64_t,
while the RHS had type uint64_t before this commit.
ASSERT_EQ(APFixedPoint::getMax(Sema).getIntPart(), Expected);
Because of the operator overloads of `APSInt`, this caused the call to
`operator==()` hidden inside ASSERT_EQ to be ambiguous between:
1) /orchestra/sources/llvm/llvm/include/llvm/ADT/APInt.h:2035:13:
inline bool operator==(uint64_t V1, const APInt &V2) { return V2 == V1; }
^
2) /orchestra/sources/llvm/llvm/include/llvm/ADT/APSInt.h:339:13:
inline bool operator==(int64_t V1, const APSInt &V2) { return V2 == V1; }
^
The ambiguity is resolved changing the type of `Expected` to `int64_t`.
|
OK, once the "APFixedPointTest.cpp: fix C++20 compilation error" thing is done, and the CI passes is OKGO. |
Since C++20, UTF8 string literals (in the form `u8"blabla"`) have type `const char8_t[N]`, which does not play nice with llvm::StringRef and llvm::StringLiteral. This commit fixes compilation of DJBTest.cpp and in C++20 explicitly converting those strings literals to regular string literals. To fix compilation of DJBTest.cpp, we had to define a new UTF8 based constructor for StringRef and one for StringLiteral. These cannot be constexpr because const char8_t * cannot be converted at compile time to const char *. With these new constructors, the StringRef(data, length) constructor becomes ambiguous if used with nullptr as first argument, such as StringRef(nullptr, 0); This was done in XCOFFObjectFile.cpp that was fixed to use the default StringRef constructor.
The exact warning was:
include/clang/AST/DeclarationName.h:210:52: error: arithmetic between different enumeration types ('clang::DeclarationName::StoredNameKind' and 'clang::detail::DeclarationNameExtra::ExtraKind') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion]
Before this patch, LLVM's DominatorTree and related templates (DominatorTree, DomTreeBuilder) did not support the computation of dominator and post-dominator trees on graphs with markers, such as GraphTraits<llvm::Inverse<T>>. They only supported plain graph traits. This patch restructures all the necessary templates, adding an additional template argument (View), which allows to compute DT and PDT on all the graphs that use GraphTraits, potentially equipped with a View. View is a marker, that allows to provide a different view on a graph that has GraphTraits. A typical example of this is llvm::Inverse. This mechanism of View markers will be used in rev.ng to implement filtered graph traits, to filter the edges of a graph implemented with GraphTraits. The new DT and PDT on the marked version of the GraphTraits will be computed on the same graphs, but treating some edges as if they are seen through the view. For instance, the Post-Dominator Tree of a graph marked with llvm::Inverse view is the Dominator Tree. The previous default behavior of DT and PDT is implemented by means of the DTIdentityView, which simply leaves the node type of the GraphTraits untouched. This is a transparent view that allows all the code in llvm to continue working as before this patch.
These tests pass successfully when running as unprivileged user, but fail if run as `root`, because the superuser has privileges to ignore file permissions. This is a known issue, reported e.g. by Gentoo as well: https://bugs.gentoo.org/775050 For now, disable these tests, since they fail in docker on our CI.
Done! See if you like the commit message better. |
Looks great. |
|
Merged in c97371c. Thanks! |
Closed
tvandera
pushed a commit
to tvandera/llvm-project
that referenced
this pull request
Oct 5, 2021
Andrei Matei reported a llvm11 core dump for his bpf program https://bugs.llvm.org/show_bug.cgi?id=48578 The core dump happens in LiveVariables analysis phase. revng#4 0x00007fce54356bb0 __restore_rt revng#5 0x00007fce4d51785e llvm::LiveVariables::HandleVirtRegUse(unsigned int, llvm::MachineBasicBlock*, llvm::MachineInstr&) revng#6 0x00007fce4d519abe llvm::LiveVariables::runOnInstr(llvm::MachineInstr&, llvm::SmallVectorImpl<unsigned int>&) revng#7 0x00007fce4d519ec6 llvm::LiveVariables::runOnBlock(llvm::MachineBasicBlock*, unsigned int) revng#8 0x00007fce4d51a4bf llvm::LiveVariables::runOnMachineFunction(llvm::MachineFunction&) The bug can be reproduced with llvm12 and latest trunk as well. Futher analysis shows that there is a bug in BPF peephole TRUNC elimination optimization, which tries to remove unnecessary TRUNC operations (a <<= 32; a >>= 32). Specifically, the compiler did wrong transformation for the following patterns: %1 = LDW ... %2 = SLL_ri %1, 32 %3 = SRL_ri %2, 32 ... %3 ... %4 = SRA_ri %2, 32 ... %4 ... The current transformation did not check how many uses of %2 and did transformation like %1 = LDW ... ... %1 ... %4 = SRL_ri %2, 32 ... %4 ... and pseudo register %2 is used by not defined and caused LiveVariables analysis core dump. To fix the issue, when traversing back from SRL_ri to SLL_ri, check to ensure SLL_ri has only one use. Otherwise, don't do transformation. Differential Revision: https://reviews.llvm.org/D97792 (cherry picked from commit 51cdb78)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is still a draft.
It's known to compile, and pass all the llvm testsuite with
orc install --test llvm, with the only pre-requisite thatorchestrais based on revng/orchestra#36revngcan be successfully built from this, but there are still some failing tests (see revng/revng#76).This patchset is still dirty: there are still some explictly reverted out-of-tree patches in the history. They are not required for building
llvm, nor forrevng, but they might still be needed for successfully building some downstream project (likerevng-c,caliban, orcold-revng). However, until allrevngtests pass, we are not able to verify this on downstream projects. When we fix therevngtests failing, I'll figure out if these pending reverted patches need to be applied (so I'll remove the revert commit), or can be dropped (so I'll leave them entirely out of the new history).@aleclearmind when you're done fixing
revngtests, I'll figure out this pending reverts issues an update the PR.