Page MenuHomePhabricator

[IRSymTab] Mark __stack_chk_guard used
ClosedPublic

Authored by ychen on Oct 26 2021, 10:34 PM.

Details

Summary

__stack_chk_guard is a global variable that has no uses before the LLVM code generation phase (how it is defined is platform-dependent). LTO needs to preserve this symbol for that reason. Currently, legacy LTO API preserves it by hardcoding the logic in Internalizer, but this symbol is not preserved by regular LTO API in thinlink phase. This patch marks __stack_chk_guard used during IR symbol table writing since this is how builtin functions are preserved by thinlink by using RuntimeLibcalls.def.

Diff Detail

Event Timeline

ychen created this revision.Oct 26 2021, 10:34 PM
ychen requested review of this revision.Oct 26 2021, 10:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2021, 10:34 PM

Can __stack_chk_fail be removed from llvm/lib/Transforms/IPO/Internalize.cpp?

Can __stack_chk_fail be removed from llvm/lib/Transforms/IPO/Internalize.cpp?

It would not be removed there. Internalize.cpp preserves both __stack_chk_fail and the guard symbol.

Can __stack_chk_fail be removed from llvm/lib/Transforms/IPO/Internalize.cpp?

It would not be removed there. Internalize.cpp preserves both __stack_chk_fail and the guard symbol.

I'm confused after reading the referenced bug, which is about doing something to be able to remove this value's hardcoding from Internalize.cpp. Does the newly added test case fail without the IRSymtab change in this patch? There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

ychen edited the summary of this revision. (Show Details)Oct 27 2021, 12:02 PM

LG

llvm/lib/Object/IRSymtab.cpp
51

Add a comma so that a follow-up doesn't need to patch this line.

llvm/test/ThinLTO/X86/builtin-nostrip.ll
31

llvm-nm sorts the symbols alphabetically by default. No need for -DAG

ychen planned changes to this revision.Oct 27 2021, 2:37 PM

handle __ssp_canary_word

ychen edited the summary of this revision. (Show Details)Oct 27 2021, 2:39 PM
MaskRay added a comment.EditedOct 27 2021, 2:42 PM

__stack_chk_guard is a user-defined variable

Can be more accurate. For some architectures glibc define the variable (https://sourceware.org/bugzilla/show_bug.cgi?id=26817)

There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

Echo this request. Would'be great to make the logic centralized, but may not be in this patch if you think not fit.

ychen edited the summary of this revision. (Show Details)Oct 27 2021, 4:47 PM
ychen updated this revision to Diff 382878.Oct 27 2021, 6:43 PM
  • Address comments
ychen added a comment.Oct 27 2021, 6:46 PM

Can __stack_chk_fail be removed from llvm/lib/Transforms/IPO/Internalize.cpp?

It would not be removed there. Internalize.cpp preserves both __stack_chk_fail and the guard symbol.

I'm confused after reading the referenced bug, which is about doing something to be able to remove this value's hardcoding from Internalize.cpp.

Apology for the confusion. I was not clear. Looking at PR28061 again, I think this patch does *NOT* fix PR28061: they are related but separate issues. (patch summary updated)
UpdateCompilerUsed/Internalizer deals with builtin symbols preservation for regular LTO using legacy LTO API; "RuntimeLibcalls.def" deals with builtin function symbols preservation for the new LTO API, but not builtin global variable like __stack_chk_guard.

Does the newly added test case fail without the IRSymtab change in this patch?

Yes, it does fail without this patch.

There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

__stack_chk_fail is listed in "RuntimeLibcalls.def". All symbols in "RuntimeLibcalls.def" is marked used already. Just added handling&test for __ssp_canary_word.

ychen added a comment.Oct 27 2021, 6:51 PM

__stack_chk_guard is a user-defined variable

Can be more accurate. For some architectures glibc define the variable (https://sourceware.org/bugzilla/show_bug.cgi?id=26817)

Yes, I've updated the summary.

There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

Echo this request. Would'be great to make the logic centralized, but may not be in this patch if you think not fit.

Hmm, I think the only place that needs this logic is IRSymTab; "RuntimeLibcalls.def" and Internalizer cover the rest of the use cases (ThinLTO, legacy LTO API).

__stack_chk_guard is a user-defined variable

Can be more accurate. For some architectures glibc define the variable (https://sourceware.org/bugzilla/show_bug.cgi?id=26817)

Yes, I've updated the summary.

There are a couple other symbols handled specially in Internalize.cpp (stack_chk_fail, and ssp_canary_word for AIX), should the all be handled uniformly?

Echo this request. Would'be great to make the logic centralized, but may not be in this patch if you think not fit.

Hmm, I think the only place that needs this logic is IRSymTab; "RuntimeLibcalls.def" and Internalizer cover the rest of the use cases (ThinLTO, legacy LTO API).

Ok I follow now. The reason why legacy (Thin)LTO and new ThinLTO work is that they invoke Internalize.cpp to perform the internalization. However, regular LTO with the new LTO API performs internalization itself based solely on the symbol resolutions which derive from IRSymTab info.

llvm/lib/Object/IRSymtab.cpp
44

I think perhaps the name of this array and the variable it is used to set (IsBuiltinFunc) ought to change.

ychen updated this revision to Diff 382912.Oct 27 2021, 9:59 PM
  • Address Teresa's comment.
ychen marked 3 inline comments as done.Oct 27 2021, 9:59 PM
ychen updated this revision to Diff 382913.Oct 27 2021, 10:01 PM
  • fix comment
This revision is now accepted and ready to land.Oct 28 2021, 7:37 AM
MaskRay accepted this revision.Oct 28 2021, 9:17 AM
This revision was landed with ongoing or failed builds.Oct 28 2021, 11:23 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review!