Page MenuHomePhabricator

[Clang][Bundler] Reduce fat object size
ClosedPublic

Authored by sdmitriev on Jan 29 2020, 9:53 AM.

Details

Summary

Fat object size has significantly increased after D65819 which changed bundler tool to add host object as a normal bundle to the fat output which almost doubled its size. That patch was fixing the following issues

  1. Problems associated with the partial linking - global constructors were not called for partially linking objects which clearly resulted in incorrect behavior.
  2. Eliminating "junk" target object sections from the linked binary on the host side.

The first problem is no longer relevant because we do not use partial linking for creating fat objects anymore. Target objects sections are now inserted into the resulting fat object with a help of llvm-objcopy tool.

The second issue, "junk" sections in the linked host binary, has been fixed in D73408 by adding "exclude" flag to the fat object's sections which contain target objects. This flag tells linker to drop section from the inputs when linking executable or shared library, therefore these sections will not be propagated in the linked binary.

Since both problems have been solved, we can revert D65819 changes to reduce fat object size and this patch essentially is doing that.

Diff Detail

Event Timeline

sdmitriev created this revision.Jan 29 2020, 9:53 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
ABataev added inline comments.Jan 29 2020, 11:49 AM
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
528–553

We usually don't do this. New types must be declared outside in anonymous namespaces. Also, better to use part-of inheritance rather than is-a here.

sdmitriev updated this revision to Diff 241269.Jan 29 2020, 1:11 PM

Addressed review comments.

ABataev added inline comments.Jan 29 2020, 1:21 PM
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
371

If you have private members, it should be a class. Also, seems to me it is a RAII class. Better to name it something like TempFileHandlerRAII or something like this.

378

I see that Contents is either None or just one byte 0. Maybe, better to use bool flag here instead of Contents?

sdmitriev updated this revision to Diff 241278.Jan 29 2020, 1:36 PM

Addressed review comments.

sdmitriev marked an inline comment as done.Jan 29 2020, 1:40 PM
sdmitriev added inline comments.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
378

Well, maybe we will need some other contents besides one zero byte in future, so I think having optional contents is probably better.

sdmitriev marked an inline comment as done.Jan 29 2020, 6:44 PM

@ABataev, do you have any additional comments?

This revision is now accepted and ready to land.Jan 30 2020, 7:53 AM
This revision was automatically updated to reflect the committed changes.