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.

As far as I can tell (there is indeed a lot of ifdeffing going on, please keep an eye on the buildbots) this change seems to be both NFC and an improvement to the previous state, so LGTM.

Could anyone please take another look at this?

Ping?

Ping

I found a clang crash that seems to be caused by this patch. Here's a reduced test case:

Sorry it took me so long to respond to this. Please take another look.

Hi Oliver,

Re-made the test asm file starting with a -O2 compilation to avoid some unnecessary code.

It's just that it seemed weird/hacky to use asm just to stick assembler directives in the middle of the generated code instead of actual assembly, so I generated an assembly version and cleaned it up a bit. I'm happy to go back to the original shorter C++ version if that's not a concern.

Added a test case that seems to work well with the existing testing setup if I rebuild LLVM with ASan enabled. I have checked out other usages of .cfi_* directives in other LLVM assembly test files and they seem to run them through some llvm tool with -triple <something including linux>, so I have added a REQUIRES: linux line to avoid breakage.

Looking into how to add a test now. I'm not familiar at all with libunwind code, so I have no idea how to cause this leak to happen from the outside (and then the test would only work when running under asan). Any pointers would be much appreciated.

FYI we have detected a 2-3% performance regression on Haswell machines in test-suite/MicroBenchmarks/ImageProcessing/Interpolation that seems to be caused by this commit.

Hm, I just realized I'm undoing https://reviews.llvm.org/D58883.

About %T not working for "process launch", what about something like RUN: %lldb -b --one-line-before-file "process launch --stdout %T/x86-zmm-write.out" -s %s %t and then FileCheck-ing?

Fixed indentation.

Removed trailing blank lines from test case, will commit now. Thanks for the review!

Added a new test that checks that, when a full path to a compiler is specified:

Thanks for the fix!

Thanks! Please let me know if it happens again and I'll try my best to debug it.

Increased wait time in test binary for TestVSCode_attach.py from 5 to 10 seconds.