Page MenuHomePhabricator

[HIP] Fix linking of asanrt.bc
ClosedPublic

Authored by yaxunl on Sep 22 2021, 9:10 PM.

Details

Summary

HIP currently uses -mlink-builtin-bitcode to link all bitcode libraries, which
changes the linkage of functions to be internal once they are linked in. This
works for common bitcode libraries since these functions are not intended
to be exposed for external callers.

However, the functions in the sanitizer bitcode library is intended to be
called by instructions generated by the sanitizer pass. If their linkage is
changed to internal, their parameters may be altered by optimizations before
the sanitizer pass, which renders them unusable by the sanitizer pass.

To fix this issue, HIP toolchain links the sanitizer bitcode library with
-mlink-bitcode-file, which does not change the linkage.

A struct BitCodeLibraryInfo is introduced in ToolChain as a generic
approach to pass the bitcode library information between ToolChain and Tool.

Diff Detail

Event Timeline

yaxunl created this revision.Sep 22 2021, 9:10 PM
yaxunl requested review of this revision.Sep 22 2021, 9:10 PM
tra added inline comments.Sep 23 2021, 9:58 AM
clang/include/clang/Driver/ToolChain.h
116–117

It appears that what we dealing with here is an on/off switch for whether we want to internalize symbols.

Perhaps it should be implemented as such.
getHIPDeviceLibs() will set the bool flag indicating *what* we want to do with the library and then the toolchain-specific code will decide *how* to make it happen. Currently it translates into specific linking option, but it may be something else. getHIPDeviceLibs does not need to know that.

The code will remain essentially the same, it's mostly the naming exercise.

yaxunl marked an inline comment as done.Sep 24 2021, 8:54 AM
yaxunl added inline comments.
clang/include/clang/Driver/ToolChain.h
116–117

will do

yaxunl updated this revision to Diff 374868.Sep 24 2021, 8:58 AM
yaxunl marked an inline comment as done.
yaxunl edited the summary of this revision. (Show Details)

Revised by Artem's comments

tra accepted this revision.Sep 24 2021, 10:02 AM
tra added inline comments.
clang/lib/Driver/ToolChains/HIP.cpp
413

Nit: I'd add /*ShouldInternalize=*/

This revision is now accepted and ready to land.Sep 24 2021, 10:02 AM
yaxunl marked an inline comment as done.Sep 24 2021, 10:16 AM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/HIP.cpp
413

will do when committing

This revision was landed with ongoing or failed builds.Sep 27 2021, 10:26 AM
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2021, 10:26 AM