Revert three more Clang patches #13181
Conversation
|
Starting build on |
Makes sense to me.
The problem is generated (synthesized) code which have no source location information. Then when you have an error resulting in multiple overloads we will need to compare their source locations to order the diagnostics. This patch makes this codes to work while the llvm_unreachable makes it crash in production.
That was probably relevant for the PCH and rootmaps? |
Is that still true? In
Maybe, but this sounds incredibly fragile. Also CMS has
Commit b71b903 mentions the command line, but this should equally go through the virtual file I mentioned above and have valid locations these days, no? |
|
Build failed on mac11/noimt. Failing tests: |
| //AXEL: llvm_unreachable("Unsortable locations found"); | ||
| assert(0 && "Unsortable locations found"); | ||
| return LOffs.first < ROffs.first; | ||
| llvm_unreachable("Unsortable locations found"); |
There was a problem hiding this comment.
Thiw would corrupt the stack if it reaches here. That's the problem can be triggered by generated code.
There was a problem hiding this comment.
Yes, I see... So you prefer to keep this patch for now? (edit: see also my argument above about initializeVirtualFile)
There was a problem hiding this comment.
I am not sure. I believe that the initializeVirtualFile is not sufficient in the cases where we generate code (via the ast synthesizers) where we pass invalid source locations. However, it might be worth to try. Let's ask cmssw.
|
@smuzaffar, can you test this PR within cmssw? |
|
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
@iarspider , @aandvalenzuela can you please test this? You need to open a PR using hahnjo:revert-clang-patches branch agaisnt the cms-sw/root root master latest branch usd by ROOT6 IBs and then start the tests for ROOT6 IBs from that PR |
|
@aandvalenzuela can you interpret the results we got from your tests for us? Is this PR good to go from cmssw standpoint? |
|
Yes @vgvassilev, it is. The three tests that failed are also failing in our integration builds. See cms-sw/cmssw#41964 ( |
|
@aandvalenzuela, thanks a lot for the confirmation! |
These were part of commit 2abc4e8 ("Mark implicit members coming from a PCH as used."). The other change to forgetDecl has already been removed in commit 853b622 ("Import LLVM r302975."), and clearing the vectors LLVMUsed and LLVMCompilerUsed does not seem needed because CodeGenerator::StartModule will swap the entire CodeGenModule.
For an optimized build, llvm_unreachable will print a warning, but if assertions are enabled it has the same effects as assert(0). As we don't see this assertion in recent times, this change is probably not relevant anymore.
I believe that all locations have a SourceManager nowadays, even from the command line.
hahnjo
force-pushed
the
revert-clang-patches
branch
from
a86f7d7 to
12cf7d2
Compare
|
Starting build on |
emitLLVMUsed: clearing the vectorsLLVMUsedandLLVMCompilerUseddoes not seemneeded because
CodeGenerator::StartModulewill swap the entireCodeGenModule.SourceManager::isBeforeInTranslationUnit: if assertions are enabledllvm_unreachablehas the same effects asassert(0). As we don't see this assertion in recent times, this change is probably not relevant anymore.SourceLocation::isBeforeInTranslationUnitThan: I believe that all locations have aSourceManagernowadays, even from the command line.