This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Fix a metadata lost issue with DICompileUnit import.
ClosedPublic

Authored by hoy on Aug 26 2020, 9:31 PM.

Details

Summary

For ThinLTO importing we don't need to import all the fields of the DICompileUnit, such as enums, macros, retained types lists. The importation of those fields were previously disabled by setting their value map entries to nullptr. Unfortunately a metadata node can be shared by multiple metadata operands. Setting the map entry to nullptr might result in not importing other metadata unexpectedly. The issue is fixed by explicitly setting the original DICompileUnit fields (still a copy of the source module metadata) to null.

Diff Detail

Event Timeline

hoy created this revision.Aug 26 2020, 9:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2020, 9:31 PM
hoy requested review of this revision.Aug 26 2020, 9:31 PM
hoy edited the summary of this revision. (Show Details)Aug 26 2020, 9:38 PM
hoy added reviewers: tejohnson, wenlei.
wenlei accepted this revision.Sep 2 2020, 8:52 AM

LGTM. @tejohnson appreciate if you could take a look too, thanks. (this is blocking an internal usage for us)

llvm/test/ThinLTO/X86/import-metadata.ll
38

Just curious, any reason for !5 = !{!4, !4} instead of !5 = !{!4}?

This revision is now accepted and ready to land.Sep 2 2020, 8:52 AM

AFAICT this looks ok, but adding @dblaikie as he is more familiar with the DI metadata. Couple minor comments below.

llvm/test/ThinLTO/X86/import-metadata.ll
10

s/metadta/metadata/

14

Should these both be the same MD value? I don't see MD1 matched anywhere else

hoy marked an inline comment as done.Sep 2 2020, 10:17 AM
hoy added inline comments.
llvm/test/ThinLTO/X86/import-metadata.ll
14

Actually MD1 comes from the current module. MD2 is from the imported module. We are checking if the imported MD2 doesn't end up having a null operand MD3. Sorry for the confusion. I should make it more explicit.

38

Good point. One !4 is just fine.

hoy updated this revision to Diff 289507.Sep 2 2020, 10:18 AM

Updating D86675: [ThinLTO] Fix a metadata lost issue with DICompileUnit import.

dblaikie accepted this revision.Sep 2 2020, 1:43 PM

AFAICT this looks ok, but adding @dblaikie as he is more familiar with the DI metadata. Couple minor comments below.

Thanks - yeah, patch description/changes/patch look right/good to me.

This revision was automatically updated to reflect the committed changes.