Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix llvm 10 #3144

Closed
wants to merge 22 commits into from
Closed

Conversation

thewilsonator
Copy link
Contributor

This fixes some compilation errors with LLVM 10.

Not fixed is the issue with change of CallSite to CallBase that changed with IRBuilder::CreateMemSet

diagnosticsFilename, EC, llvm::sys::fs::F_None);
if (EC) {
irs.dmodule->error("Could not create file %s: %s",
diagnosticsFilename.c_str(), EC.message().c_str());
fatal();
}

//FIXME: this has been replaced by setDiagnosticHandler
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N.B. I'm not sure how tot get back the original behaviour.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have a link to the commit that removed setDiagnosticsOutputFile ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that was purely speculative as it appears that setDiagnosticsOutputFile no longer exists and setDiagnosticHandler is the only thing there that could logically replace its functionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gen/modules.cpp Outdated Show resolved Hide resolved
@thewilsonator
Copy link
Contributor Author

@Robertorosmaninho if you are trying to build with the latest LLVM you'll need to use this patch. You'll also need to comment out this line as I haven't managed to work out a solution for this CallSite use.

@Hardcode84
Copy link
Contributor

I don't like make_unique ifdefs copypaste everywhere but I don't see a good solution here. We can write our make_unique wrapper with ifdefs but it will be bigger than actual make_unique implementation :)

@thewilsonator
Copy link
Contributor Author

Neither do I (I wanted to put this up sooner so that @Robertorosmaninho can have a functioning LDC to start his work on).

I could have sworn we used to have an llvm compatibility module somewhere but I couldn't find it, otherwise I'd have put it in there. I think we are going to have to have something like that for the CallSite changes anyway, those look pretty nasty.

gen/modules.cpp Outdated Show resolved Hide resolved
@@ -29,6 +29,12 @@
#include "lld/Common/Driver.h"
#endif

#if LDC_LLVM_VER >= 1000
using std::make_unique;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this depend on C++ version? LLVM 10 requires C++14?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as kinke already mentioned, this breaks with c++14 compilation for LLVM < 10. So change the condition to cplusplus version, instead of LLVM version.

@@ -34,6 +34,11 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MD5.h"

#if LDC_LLVM_VER >= 1000
using std::make_unique;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's move this repeated piece into a helper header file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I swear we used to have an LLVM compatibility file somewhere, did it get removed? Because I can't find it.

@JohanEngelen
Copy link
Member

@thewilsonator Can you track down which LLVM commit / phabricator removed removeCallEdgeFor?

gen/modules.cpp Outdated Show resolved Hide resolved
thewilsonator and others added 4 commits August 30, 2019 08:01
"createAddressSanitizerModulePass" is outdated, new version is "createModuleAddressSanitizerLegacyPassPass" from llvm/Transforms/Instrumentation/AddressSanitizer.h
@thewilsonator
Copy link
Contributor Author

@Robertorosmaninho you'll also need to apply druntime.txt to druntime to get the runtime to build.

@thewilsonator
Copy link
Contributor Author

Any guesses as to why MSVC think that the call to make_unique is ambiguous? are we using std, or llvm somewhere in the includes?

@kinke
Copy link
Member

kinke commented Aug 31, 2019

Any guesses as to why MSVC think that the call to make_unique is ambiguous? are we using std, or llvm somewhere in the includes?

Nope, no relevant using namespace std or using std::make_unique here. The problem is apparently argument-dependent lookup, i.e., std::make_unique is found automatically because we pass a std::error_code argument (and now use an unqualified make_unique name), so that the std namespace is searched too (and MSVC has had std::make_unique for ages).
Don't blame MSVC :] - this is clang v8.0.1. ;)

@JohanEngelen
Copy link
Member

my guess is that we enable c++11 (not 14), which does not have std::make_unique and ADL doesn't trigger. Clang being incorrect here doesn't sound likely to me.

@kinke
Copy link
Member

kinke commented Aug 31, 2019

MS cl (from VS 2019) behaves identically, so the clang-cl wrapper is correctly compatible here. The MS compiler doesn't support C++11 anymore: /std:<c++14|c++17|c++latest> and defaults to C++14.

So this apparently just shows that this PR can currently only be compiled as C++11 and not with any later version (with LLVM < 10). Edit: And we seem to just inherit the C++ lang version from the LLVM C++ flags.

@kinke
Copy link
Member

kinke commented Aug 31, 2019

Microsoft is obviously a PITA and chose to use 199711 for predefined __cplusplus. So for MSVC, one should check _MSVC_LANG (= 201402) instead.

kinke added a commit to kinke/ldc that referenced this pull request Sep 20, 2019
Include some of Nicholas' fixes in ldc-developers#3144.
kinke added a commit to kinke/ldc that referenced this pull request Sep 21, 2019
Including some of Nicholas' fixes in ldc-developers#3144.
kinke added a commit to kinke/ldc that referenced this pull request Sep 21, 2019
Including some of Nicholas' fixes in ldc-developers#3144.
kinke added a commit to kinke/ldc that referenced this pull request Sep 24, 2019
Including some of Nicholas' fixes in ldc-developers#3144.
kinke added a commit to kinke/ldc that referenced this pull request Sep 25, 2019
Including some of Nicholas' fixes in ldc-developers#3144.
@kinke
Copy link
Member

kinke commented Oct 18, 2019

This has been incorporated as #3166 and #3187, right?

@JohanEngelen
Copy link
Member

yeah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants