Some more information about the problem. The stack trace looks like follows. The problem manifests as an ASan out of memory error because the move constructor in varlen_sort.h:59 is getting incorrect data and it's trying to allocate an absurdly big amount of memory.

Removed unnecessary parentheses.

Looks good to me.

After this patch, llvm-dis will crash if passed a bitcode file with metadata that is now non-representable.

Yes, it fixes both the original compilation where we noticed the problem
and the reduced test case. Thank you very much for the fix and the quick
response!

I've been experimenting a little bit and it seems that I can avoid the deadlocks I was seeing by applying only the modification to TypeCategoryMap::Add. This would avoid the problem that @hawkinsw pointed out in my change to FormatManager::GetCategoryForLanguage. However, the following problem could still happen with the change to TypeCategoryMap::Add:

Thanks for the reply! Yes, I've seen some test failures while trying to update our llvm to (closer to) HEAD, and I bisected them to this commit. These were the affected tests:

Is it possible that this change just broke the lldb data formatter for string?

By the way, the line number for llvm::misexpect::verifyMisExpect in the stack trace above includes the debug printfs I inserted to get the variable values so it won't match the exact line number in the repo. But I think that's the only call to BranchProbability in that function so I hope it's not very confusing anyway. Sorry about that.

Hi, this patch is causing floating point exceptions for us during profile generation. On a debug build I get this assertion failure (see stack trace below):

clang: /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/BranchProbability.cpp:41: llvm::BranchProbability::BranchProbability(uint32_t, uint32_t): Assertion `Denominator > 0 && "Denominator cannot be 0!"' failed.

We've observed that this patch introduces infinite loops in some cases. Here's a reduced test case:

export class Foo extends Bar {
  get case(): Case {
    return (
        (this.Bar$has('case')) ? (this.Bar$get('case')) :
                                 (this.case = new Case()));
  }
}

We've found segfaults in lld when linking go binaries after this patch:

This breaks some existing code (we have found this issue building the JPEG XL library at github.com/libjxl). This is a very simplified version of the problem:

I rebuilt clang at this commit with -DLLVM_USE_SANITIZER=Address and it reports a problem nearby. I don't know if this patch is the actual root cause for the crash or if it's triggering an existing bug. Here's the full report in case it can help you debug the issue further:

Hi, I created the reproducer that akuegel posted above. I have reproduced the crash starting with a clean repo with the steps below. The stack trace is a bit different, but a release build following the same steps will get you a stack trace very similar to what he posted. What I've noticed is that the crash does not always reproduce consistently. I was trying a new release build to double check the steps below, I ran the build command multiple times in a row, and it crashed only sometimes. The crash is a segfault, so it's possible that sometimes an invalid memory access doesn't immediatly crash the program.

This change is causing asan errors when running the clang-tidy checks under ASan, most likely because of the reason akuegel pointed out in his comment. I'm going to revert the patch to unbreak HEAD until the problem can be fixed.

In D99980#2738628, @ABataev wrote:

Must be fixed in 13a51e017c09ce449ba2ec0024baf356d6dfcbad

Hi, we're seeing some build failures after this patch. If you build the following reduced test case with clang -cc1 -emit-obj -O3 -vectorize-slp reduced.cc

Hi, one of our tests is failing because of this patch and it looks like an actual bug. clang-format now turns this file:

import './a';
import {bar} from './a';

into this:

barimport './a';

I'm going to go ahead and commit this given that I have just made the last suggested modification and the patch has already been up for review without further comments for a long while (sorry for the late replies, work keeps getting in the way of work).

Changed logic that checked the type of unit to use header.IsTypeUnit() as suggested by reviewer.

Moved index handling to DWARFUnit.cpp as suggested by Pavel.

Thanks for accepting the patch! (and sorry for the late reply, I was on vacation). I'll commit this now :)

Hi! We have bisected a clang crash to this patch. The following reduced test case crashes clang when built with clang -O1 -frounding-math:

Found a crash that seems to be caused by this patch. This is a reduced test case:

extern "C"
#define a(b, c, args) d(double, b, , args)
#define d(g, b, c, args) g b args
    a(sqrt, , (double));
int e;
double f = sqrt(e);

Building with clang -target powerpc64le-grtev4-linux-gnu -ffast-math repro.cpp crashes a top-of-tree clang.

Thanks everyone for the reviews! I'm committing this now.

then the check Rows[H].getSignature() != S won't fail

The condition fails. It should return nullptr but returned a Rows entry.

Updated diff addressing the new round of feedback.

Addressed reviewer comments.

Thanks for the comments!

Changed test expectation from "not a stack dump" to the actual output from llvm-symbolizer for the test case.

LGTM

This looks good to me FWIW. I'd prefer if someone more experienced with the project would accept it, though.

Thanks for the note about the flags. We have tried both of them, and any (or both) of them being present causes the regression to go away, so that unblocks us while we work on producing a proper test case.

We're seeing some large performance regressions on Eigen (http://eigen.tuxfamily.org/) running on haswell and skylake machines with this patch. Could you please revert it?

Thanks for doing the extra work of adding a test. Just one final nit.

I think it looks good now. My only issue is that it seems to rely on a couple of glibc-specific features: glibc modifying the fields adds and subs in dl_phdr_info when loading/unloading libraries (which this patch uses to know when to invalidate the cache), and dl_iterate_phdr holding a lock (which the patch relies on to avoid races while accessing the cache). What other libc implementations do we support? Do they share these behaviors we rely on here?

Thanks!

Looks good to me.