Page MenuHomePhabricator

Emit S_OBJNAME symbol in CodeView
Needs ReviewPublic

Authored by aganea on Feb 6 2018, 7:33 PM.

Details

Summary

We've had a longstanding incompatibility with MSVC where we don't emit a couple of CodeView records that normally go in each file. This patch aims to address the first of these records - the S_OBJNAME symbol. This symbol is very simple. It just contains the full path to the object file that the debug info is being emitted to.

When building from clang, the approach taken here is to use the target file name, as decided by the CompilerInstance::createDefaultOutputFile. It is then passed down to the backend through MCTargetOptions, to make it available to CodeViewDebug. The same approach is taken when using llc or LTO/ThinLTO (through LLD or llvm-lto2). When emitting to stdout (path is '-'), we emit the S_OBJNAME record with an empty name.

The objective of this patch is to support live code patching tools like Recode or Live++. They are different from Microsoft Edit & Continue, which we don't plan to support. Several changes are needed in LLVM to support these tools:

  1. Emit S_OBJNAME (this patch)
  2. Emit full cc1 cmd-line in LF_BUILDINFO, see D80833 (needs to be fixed & re-landed).
  3. Emit two-byte nops, see D81301.
  4. Emit the 'hotpatch' flag in S_COMPILE3.

Diff Detail

Unit TestsFailed

TimeTest
380 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
340 mslinux > HWAddressSanitizer-x86_64.TestCases/Linux::reuse-threads.cpp
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions -mllvm -hwasan-instrument-stack=0 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/Linux/reuse-threads.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Linux/Output/reuse-threads.cpp.tmp && env HWASAN_OPTIONS=disable_allocator_tagging=1:random_tags=0:verbose_threads=1 /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Linux/Output/reuse-threads.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/Linux/reuse-threads.cpp
210 mswindows > lld.ELF/invalid::symtab-sh-info.s
Script: -- : 'RUN: at line 4'; c:\ws\w64\llvm-project\premerge-checks\build\bin\yaml2obj.exe --docnum=1 C:\ws\w64\llvm-project\premerge-checks\lld\test\ELF\invalid\symtab-sh-info.s -o C:\ws\w64\llvm-project\premerge-checks\build\tools\lld\test\ELF\invalid\Output\symtab-sh-info.s.tmp.o

Event Timeline

zturner created this revision.Feb 6 2018, 7:33 PM
rnk added a reviewer: aprantl.Feb 6 2018, 8:35 PM

I'd back out the C API changes, but otherwise this makes sense to me. Let's check with @aprantl, though.

llvm/include/llvm-c/DebugInfo.h
202–203 ↗(On Diff #133138)

I'd suggest leaving this alone, we can add a LLVMDIBuilderCreateCompileUnit2 if any frontend using the C API ever cares about this.

Whenever changing IR / IR Metadata there also needs to be a Bitcode upgrade test in test/Bitcode with a .bc file of the old format, and an assembler round-trip test in test/Assembler.

That said, I'm wondering if this isn't a property that is better on the llvm::Module. For example, in an LTO compilation, do you really want the name of the individual bitcode-holding .o files or do you want the name of the lto.o file? Another example is the Swift frontend, which always generates at least DICompileUnits per llvm::Module. It would be more efficient to put, e.g., a NamedMDNode into the module so this information can be shared between the CUs.

Does this need to be the name of an actual object file? Any idea how this should work in the case of LTO?

For the .dwo file name, which must be the actual name of the final .dwo file (ie: intermediate IR 'object' file names are not relevant, only the final true object file because that's where the .dwo file comes from) this wasn't passed down through debug info metadata (because at the time the metadata's generated the final object file name is not known - that metadata might be merged, split, Thin merged, etc, before the final object is emitted) but through the MCOptions struct passed into LLVM's MC layer.

Does this need to be the name of an actual object file? Any idea how this should work in the case of LTO?

For the .dwo file name, which must be the actual name of the final .dwo file (ie: intermediate IR 'object' file names are not relevant, only the final true object file because that's where the .dwo file comes from) this wasn't passed down through debug info metadata (because at the time the metadata's generated the final object file name is not known - that metadata might be merged, split, Thin merged, etc, before the final object is emitted) but through the MCOptions struct passed into LLVM's MC layer.

I tested with MSVC LTCG (their name for LTO) and here's the behavior it exhibits.

  1. When compiling with LTCG and outputting foo.obj, the output file is not recognized as a COFF object file. I believe this matches our behavior of outputting bitcode files.
  2. When linking 2 LTCG object files together and getting a final PDB, the object name symbols reference the "pseudo-object files" mentioned in step 1.

I believe the reason for this behavior is that the final PDB contains information about every original compiland, regardless of whether it participated in LTO. I mentioned in the original patch description that the S_OBJNAME symbol is 1 out of 2 symbols that we don't yet emit. The other one emits more info including (among other things) the full command line used to generate the object file. So I think the idea here is that with all of this information combined, given only a PDB a person can reproduce the exact build. For that reason I think this needs to be the original bitcode-holding .o files.

Adrian - what do you think about Reid's point regarding not changing the C API unless needed? I'm fairly OK with that, but not sure. Seems like it could be a surprising "gotcha" where someone thinks they're doing the right things to produce CodeView but they're missing this because the API isn't exposed/suggesting that usage.

It would be more efficient to put, e.g., a NamedMDNode into the module so this information can be shared between the CUs.

I assume the string data itself is shared by the bitcode format? But I don't really know.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
814–819

This is O(N^2) (erase is O(N)) & could be made O(N) (using a technique similar to the erase+remove idiom - using literal erase+remove probably isn't practical due to the stateful nature of the walk).

Is there no other logic for this already elsewhere in LLVM?

832–833

Probably best just to use another/separate declaration here? (I think such a thing might be specified in the style guide, but I'm not sure)

It would be more efficient to put, e.g., a NamedMDNode into the module so this information can be shared between the CUs.

I assume the string data itself is shared by the bitcode format? But I don't really know.

All of this code is new to me, so I'm still kind of fumbling along as I learn how all this stuff works together, but my impression was that Adrian's suggestion only really applied if I wanted the name of the lto.o file. Given that I need the name of the bitcode containing .o file, does any of this still apply? Is the current solution appropriate in that case?

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
814–819

Unfortunately there's not. But you're right, I can do this a bit more efficiently by keeping a separate read pointer and write pointer.

Adrian - what do you think about Reid's point regarding not changing the C API unless needed? I'm fairly OK with that, but not sure. Seems like it could be a surprising "gotcha" where someone thinks they're doing the right things to produce CodeView but they're missing this because the API isn't exposed/suggesting that usage.

This depends on whether this is just a nice-to-have feature for Windows or if it is important enough to warrant creating a new API function.

It would be more efficient to put, e.g., a NamedMDNode into the module so this information can be shared between the CUs.

I assume the string data itself is shared by the bitcode format? But I don't really know.

I think that is correct.

There is still the question on how this is supposed to behave in an LTO build? Do you want the name of the shared LTO.o file for all CUs, or do you want to preserve the names of the bitcode-.o files? If the former is the case, then it makes more sense to make this a property of the Module, otherwise it needs to be on the DICompileUnit.

aprantl added inline comments.Feb 7 2018, 10:36 AM
clang/lib/CodeGen/CGDebugInfo.cpp
567 ↗(On Diff #133138)

Should this not be gated on the debuginfo kind being codeview?

This depends on whether this is just a nice-to-have feature for Windows or if it is important enough to warrant creating a new API function.

I can back out the api changes, it doesn't seem important enough to me until there is at least 1 active user of the c api who wants it.

There is still the question on how this is supposed to behave in an LTO build? Do you want the name of the shared LTO.o file for all CUs, or do you want to preserve the names of the bitcode-.o files? If the former is the case, then it makes more sense to make this a property of the Module, otherwise it needs to be on the DICompileUnit.

I mentioned in another response but it might have been overlooked. It needs to be the bitcode-.o files.

clang/lib/CodeGen/CGDebugInfo.cpp
567 ↗(On Diff #133138)

Couldn't a non-CodeView processor just ignore this field if they aren't interested in it? I can't think of a good reason to gate it at this high of a level.

There is still the question on how this is supposed to behave in an LTO build? Do you want the name of the shared LTO.o file for all CUs, or do you want to preserve the names of the bitcode-.o files? If the former is the case, then it makes more sense to make this a property of the Module, otherwise it needs to be on the DICompileUnit.

I mentioned in another response but it might have been overlooked. It needs to be the bitcode-.o files.

Thanks.

Should this not be gated on the debuginfo kind being codeview?

Couldn't a non-CodeView processor just ignore this field if they aren't interested in it? I can't think of a good reason to gate it at this high of a level.

It needlessly increases the size of the intermediate products in the LTO case on non-windows platforms and every time we encode any paths in Clang build products we are inviting problems with non-reproducible builds.
Related question: does remapDIPath do the right thing for the *output* file?

zturner updated this revision to Diff 133309.Feb 7 2018, 2:31 PM

Gated the passing of this flag behind a check for CodeView. I also removed the O(N^2) function anyway in favor of just calling make_absolute. This seems more correct anyway as it will handle the case where the output file is specified as an absolute path already.

I think this is still missing the bitcode upgrade test.

llvm/test/DebugInfo/COFF/buildinfo.ll
42 ↗(On Diff #133309)

Please strip all quoted attributes., they are bound to break in future versions

I was about to say "please make sure this honors -fdebug-prefix-map" but I suppose codeview probably contains so many absolute paths at the moment that we should instead have a concerted effort to make debug info cwd-independent at some later point instead.

zturner updated this revision to Diff 133446.Feb 8 2018, 10:31 AM

Adds tests for bitcode upgrade and assembly roundtripping (as well as the necessary fixes to make these tests pass).

aprantl added inline comments.Feb 8 2018, 10:42 AM
clang/lib/CodeGen/CGDebugInfo.cpp
561 ↗(On Diff #133446)

You don't need to create a local copy of the string here. You could just use a ?: in the createCompileUnit invocation.

llvm/test/Assembler/cu-outputfile.ll
1 ↗(On Diff #133446)

We usually do llvm-as < %s | llvm-dis - | llvm-as -| llvm-dis - | FileCheck.

llvm/test/Bitcode/upgrade-cu-outputfile.ll
2 ↗(On Diff #133446)

There should be a .bc file created by an llvm-as from before this patch checked into the repository and that should be used as input.

zturner updated this revision to Diff 133454.Feb 8 2018, 11:03 AM

Modified the tests as suggested by @aprantl

The only thing that's missing is a test for the correct hashing of the new field in unittests/IR/MetadataTest.cpp.

To clarify my last comment: No need to check for the correct hashing, since DICompileUnits are always distinct, but extending DICompileUnitTest would be the right thing to do.

rnk added a comment.Oct 29 2018, 5:45 PM

I was reviewing the records that msvc emits that clang does not, and I noticed this again.

Here's a thought. Why are we putting this on the compile unit in the first place? Why not use a global named metadata node? Most of the complexity in this patch is from going through DICompileUnit, when we can just claim a global named metadata node for the output object file. It could probably even point to a DIFile so we can preserve some information about whether the path is relative.

In D43002#1279796, @rnk wrote:

I was reviewing the records that msvc emits that clang does not, and I noticed this again.

Here's a thought. Why are we putting this on the compile unit in the first place? Why not use a global named metadata node? Most of the complexity in this patch is from going through DICompileUnit, when we can just claim a global named metadata node for the output object file. It could probably even point to a DIFile so we can preserve some information about whether the path is relative.

If this is meant to name the final object file that's emitted - perhaps it shouldn't be in metadata at all. The Split-DWARF DWO file name is similar: it must name the actual file that was emitted - and isn't related to the original modules that may've been generated. This data is passed down through an MCOption, if I recall correctly.

aganea added a subscriber: aganea.Oct 30 2018, 5:15 AM

@zturner Do you mind if we took over this patch and finish it? This prevents using Live++ with Clang.

Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2020, 5:58 AM

Hi, I just jumped on this project. I'm currently trying to get S_OBJECTNAME symbol in order to make Live++ work.
I was looking at the feed and I don't understand what you mean @rnk when you say "Why not use a global named metadata node?". Could you please give more clarification on what you mean here, it would help a lot.
Also, @dblaikie if the info shouldn't be in metadata, where do you think it should be ?
Thanks

rnk added a comment.Mar 10 2020, 10:27 AM

Thinking about it again, maybe it should be an MCOption like David suggested. S_OBJNAME should probably refer to the final location of the native object file, which is only known to whoever sets up MCOptions. For example, with "fat" LTO, the linker is the one responsible for setting up the output file.

Named metadata nodes are things like !llvm.ident = {...} that you see at global scope in .ll files, but I don't think it's relevant.

As far as this patch is concerned, I don't think we should put this thing on the compile unit. The whole idea of compile units is to preserve the separation between compilation units through LTO, but we can't really do that in codeview anyway.

Hello, I as requested placed the outputfile name in the MCTargetOptions.h. However, I encountered an issue. @rnk

I didn’t find any way to go back to MCTargetOptions.h from the CompilerInvocation.cpp. I understand it would make sense that there is one somewhere, but I can’t find it. Do any of you know how I’m supposed to access the MCTargetOption? Should I create my own way there?
Sorry to waste your time and maybe the answer is simple to answer but I’m fairly new to this code so there is some aspects of the code I dont fully understand.

aganea commandeered this revision.May 28 2020, 12:00 PM
aganea added a reviewer: zturner.
aganea edited the summary of this revision. (Show Details)May 28 2020, 12:19 PM
aganea added reviewers: MaskRay, reames.
aganea added a subscriber: lantictac.
aganea updated this revision to Diff 266989.May 28 2020, 12:27 PM

I'm taking over this patch, as discussed offline with Zachary.

I've re-implemented the patch to use the target/final file name of the .OBJ, as suggested.

The bulk of the changes is mostly to pass around the file name, down to the back end.

First question:

Since split dwarf has to do some similar things can we not use the same support? This seems to be a lot of changes for this.

Second question:

and if we can't use the same support can we make split dwarf use this? Having two separate methods for passing everything around is a bit painful.

aganea updated this revision to Diff 267111.EditedMay 28 2020, 7:34 PM

Simplified. Now using CodeGenOptions to pass the object filename around.

aganea added a comment.EditedMay 28 2020, 7:39 PM

First question:

Since split dwarf has to do some similar things can we not use the same support? This seems to be a lot of changes for this.

Second question:

and if we can't use the same support can we make split dwarf use this? Having two separate methods for passing everything around is a bit painful.

To answer both questions, I tried to converge but the uses are slightly different. In one case (DWARF) the -split-dwarf-file/-split-dwarf-output flags are passed down almost blindly to the back-end, whereas for COFF there's more logic to determine where the file should be emitted (see CompilerInstance::createOutputFile). We always need full path names in S_OBJNAME, whereas (it seems) split DWARF can handle relative paths (for example see llvm-mc -split-dwarf-file). I think the purpose is also different: the .dwo is for immediate debugger consumption, whereas the path in S_OBJNAME seems to be used more like a global identifier.

Please let me know if you feel this could be done differently.

reames resigned from this revision.May 29 2020, 11:49 AM
aganea updated this revision to Diff 304364.Tue, Nov 10, 5:54 PM
aganea edited the summary of this revision. (Show Details)
aganea edited reviewers, added: mstorsjo; removed: reames, espindola.

Simplify.
Added coverage when using clang-cl since this is a MSVC-specific feature.
When -fdebug-compilation-dir=. is used, relative paths are now emitted in the S_OBJNAME record.