Thanks for working on this!

Hi, do we know whether this patch fully mitigated the issue or if there are still opportunities to improve compile time? I'm using clang trunk from Wednesday March 20th, and I cannot finish the compilation of a large project with ThinLTO and autofdo profile (compilation time was reasonable before, but ballooned to over 10 hours). When I run perf top to see what is happening, I see that the compiler is most of the time in code dealing with dominator trees or in CodeGenPrepare::runOnFunction. I don't remember seeing this issue before (Clang from February). A snapshot of hottest functions while waiting for the never finishing linking:

Eli's suggestions

Thanks for your feedback, Eli

I definitely understand how the diagnostic can be confusing. However, it's the same diagnostic gcc provides too, so gcc users wouldn't be surprised. But you're right this can be improved by at least mentioning the attribute used in the diagnostic message. There's no bug report to this yet, it would be nice to file it.

I'm not an expert in swift either. I'll wait for @davezarzycki input on what's happening, but I suspect the code base is indeed incompatible with gcc due to the nature of the error they experienced. This testcase is creduce-d from what I observed in swift.

Both approaches make sense to me. I'll re-land the previous patch in favor of gcc compatibility, since the semantics of attribute used in member functions of class templates were first implemented in gcc.

That’s fine by me.

Hi davezarzycki, thanks for reporting this! This is not expected, I'll revert this diff for now. Sorry for this.

Thanks for reviewing!

Rebase + Aaron's suggestions. Thanks for the suggestions!

No problem, thanks for your suggestions!

Removing facebook tags

@MaskRay this was originally reported here: https://github.com/facebookincubator/BOLT/issues/23

Thanks for reviewing!

Thank you Rafael and Jonas for reviewing

Removing the if

Implement suggestions

This is static because the function DisassembleObject potentially runs multiple times. So the static is to avoid re-creating the same set for each object, and it's also why it checks if it is empty.

Thanks everyone that helped to review this, I appreciate your help.

Change the test case input from binary to YAML.

Address comments.

Address rafael's comments

Addressing rafael's comments

Sorry I just saw rafael's comments, I'll address them too.

Address comments.

The fact that DataExtractor already has read*LEB128 is an important
point. So I decided to move the only missing helper,
"readEncodedPointer", into DataExtractor. However, it is DWARF-specific.
So I've put it into DWARFDataExtractor and made the necessary
adjustments. I've addressed your other concerns as well, let me know if
this is fine.

Thanks Adrian, I'll drop the \brief. That makes sense, thanks for the context on DataExtractor versus other approaches. In our client code, it never made a lot of sense to use DataExtractor and that's part of the reason I was lifting DWARF decoding helpers to live on their own. Then I was using readobj as a proxy for our client code so we have something upstream that demonstrates how we are accessing and manipulating DWARF CFIs (both coming from FDEs and CIEs). Then I'll take a second look on this design to avoid pitfalls with unchecked bounds.

Add a more complete LIT file that tests all opcodes

np, thanks for reviewing this!