This is an archive of the discontinued LLVM Phabricator instance.

Emit .debug$H section in clang
ClosedPublic

Authored by zturner on Dec 6 2017, 12:59 PM.

Details

Summary

This causes emission of .debug$H section unconditionally when -gcodeview is present.

I ran some benchmarks on a self-hosted build and found a ~2% increase in overall build time (within the margin of error, so basically noise), and a ~15% increase in object file size, on average.

Currently the linker doesn't use this, that will work will come in a followup patch. For now, we're just testing that clang emits it correctly with some obj2yaml and llc based tests.

Diff Detail

Event Timeline

zturner created this revision.Dec 6 2017, 12:59 PM
compnerd edited edge metadata.Dec 8 2017, 11:34 AM

I really wish that we had some way to indicate that this is a LLVM extension.

llvm/include/llvm/DebugInfo/CodeView/TypeHashing.h
59

Do you mind using Algorithm. I really don't like the abbreviation, especially since the name is already fairly lengthy.

rnk added a subscriber: hans.Dec 13 2017, 10:48 AM

I think we'll need some flag for this. A 15% object file size increase is quite a bit, and it will probably slow down linking with MSVC, which is what most Chromium developers are using today. I suspect you'll want to land this, and then @hans will want to roll clang while you're on vacation, and there is some risk that the larger object files will cause problems and he'll need to revert it.. A hidden cl::opt/-mllvm flag would be fine for this, and we can add a real clang flag later (it would add a module flag like /guard:cf).

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
494

I'd check the cl::opt here.

564–566

I bet we can remove this check now that we have the if (TypeTable.empty()) return; line. Want to give it a try here and above in emitTypeInformation?

llvm/test/DebugInfo/COFF/global-type-hashes.ll
1

I'd do an assembly emission test as well, just to get code coverage of the verbose asm comment code. You only really need to look for .debug$H and a few hashes after that.

hans added a comment.Dec 13 2017, 11:28 AM
In D40917#954173, @rnk wrote:

I think we'll need some flag for this. A 15% object file size increase is quite a bit, and it will probably slow down linking with MSVC, which is what most Chromium developers are using today. I suspect you'll want to land this, and then @hans will want to roll clang while you're on vacation, and there is some risk that the larger object files will cause problems and he'll need to revert it.

+1 A flag would be great, and we can enable it by default for Chromium's win/lld builds to get coverage without potentially disrupting developers using link.exe.

zturner updated this revision to Diff 126803.Dec 13 2017, 12:00 PM

Added a command line option and an assembly test.

rnk accepted this revision.Dec 13 2017, 1:38 PM

lgtm!

This revision is now accepted and ready to land.Dec 13 2017, 1:38 PM
This revision was automatically updated to reflect the committed changes.