Page MenuHomePhabricator

[lld] Explicitly ignore comdat groups when parsing LTO object
ClosedPublic

Authored by sbc100 on Jun 4 2019, 4:47 PM.

Details

Summary

Any symbols defined in the LTO object are by definition the ones we
want in the final output so we skip the comdat group checking in those
cases.

This change makes the ELF code more explicit about this and means
that wasm and ELF do this in the same way.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Jun 4 2019, 4:47 PM
sbc100 added a reviewer: ruiu.Jun 4 2019, 4:47 PM
ruiu accepted this revision.Jun 4 2019, 7:15 PM

LGTM

lld/ELF/InputFiles.cpp
623 ↗(On Diff #203048)

80 col?

This revision is now accepted and ready to land.Jun 4 2019, 7:15 PM
thakis added inline comments.
lld/ELF/Driver.cpp
1506 ↗(On Diff #203048)

fyi, the usual style is /*IgnoreComdats=*/true (with no space and a =): for true it doesn't make a big difference, but for false it's not clear without the = if the comment means "set IgnoreComdats to false, that is they are not ignored" or "passing false here _means_ IgnoreComdats, that is passing false means they _are_ ignored".

sbc100 updated this revision to Diff 203199.Jun 5 2019, 10:35 AM
  • clang-format
  • feedback
This revision was automatically updated to reflect the committed changes.

We are trying to migrate from gold to lld, and we found that when building with thinlto, lld does not de-duplicate .debug_types. De-duplication is successful with monolithic lto. Repro: clang -flto=thin -fdebug-types-section -fuse-ld=lld -Wl,-plugin-opt=-generate-type-units

If we set ignoreComdats=false for lto builds, then the de-duplication is successful again. Is the assumption "any symbols defined in the LTO object are by definition the ones we want in the final output" only true for monolithic lto?

wenlei added a subscriber: wenlei.Tue, May 19, 12:28 PM

We are trying to migrate from gold to lld, and we found that when building with thinlto, lld does not de-duplicate .debug_types. De-duplication is successful with monolithic lto. Repro: clang -flto=thin -fdebug-types-section -fuse-ld=lld -Wl,-plugin-opt=-generate-type-units

If we set ignoreComdats=false for lto builds, then the de-duplication is successful again. Is the assumption "any symbols defined in the LTO object are by definition the ones we want in the final output" only true for monolithic lto?

You could be right here. I'm don't understand thingLTO enough to know that answer to that question of the top of my head. However the behavior before and after this change should be the same, since I believe they were being ignored before this change too. Do you see any other side effects of settings this to false?

christylee added a comment.EditedTue, May 19, 12:43 PM

However the behavior before and after this change should be the same, since I believe they were being ignored before this change too

I tried an earlier linker version without this patch and lld also did not de-duplicate. My guess is since with thinlto we are not doing true whole program analysis, there's still some leftover comdat groups we need to clean up at link time.

Do you see any other side effects of settings this to false?

I don't think so. My understanding is that in the monolithic lto case, if we set this to false then no extra comdat sections will be removed. If we set it to true for thinlto, lld will then link as normal.

Let me put up a diff to set ignoreComdats to false for thinlto and request more comments.

We are trying to migrate from gold to lld, and we found that when building with thinlto, lld does not de-duplicate .debug_types. De-duplication is successful with monolithic lto. Repro: clang -flto=thin -fdebug-types-section -fuse-ld=lld -Wl,-plugin-opt=-generate-type-units

If we set ignoreComdats=false for lto builds, then the de-duplication is successful again. Is the assumption "any symbols defined in the LTO object are by definition the ones we want in the final output" only true for monolithic lto?

+@dblaikie

hoyFB added a subscriber: hoyFB.Tue, May 19, 2:45 PM