Page MenuHomePhabricator

Default to disabling the libunwind frameheader cache
ClosedPublic

Authored by saugustine on Aug 18 2020, 12:08 PM.

Details

Summary

Although it works fine with glibc, as currently implemented the
frameheader cache is incompatible with certain platforms with
slightly different locking semantics inside dl_iterate_phdr.

Therefore only enable it when it is turned on explicitly with
a configure-time option.

Diff Detail

Event Timeline

saugustine created this revision.Aug 18 2020, 12:08 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 18 2020, 12:08 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
saugustine requested review of this revision.Aug 18 2020, 12:08 PM
emaste added a subscriber: emaste.Aug 18 2020, 1:20 PM

Looks reasonable to me.

For reference here's a FreeBSD review for dl_iterate_phdr: provide exclusive locking for callback when statically linked.

emaste added a subscriber: dim.Aug 18 2020, 1:28 PM

Ah if you use _LIBUNWIND_USE_FRAME_HEADER_CACHE instead (i.e., a _ between FRAME and HEADER) it will match the workaround committed to FreeBSD.

jgorbe accepted this revision.Aug 18 2020, 1:40 PM

LGTM

Rework to match bsd workaround.

Now just waiting on a libunwind group reviewer.

mstorsjo accepted this revision.Aug 18 2020, 2:29 PM
mstorsjo added a subscriber: mstorsjo.

So, this disables code that currently is enabled, unless it's explicitly requested to be enabled? Sounds good to me, but the subject line sounds to me like it allows enabling something that currently is disabled.

LGTM with the subject line rephrased.

This revision is now accepted and ready to land.Aug 18 2020, 2:29 PM

Update for comment.

saugustine retitled this revision from Allow enabling the libunwind frameheader cache at configure time. to Default to disabling the libunwind frameheader cache.Aug 18 2020, 2:36 PM
This revision was landed with ongoing or failed builds.Aug 18 2020, 2:37 PM
This revision was automatically updated to reflect the committed changes.

Should we cherry-pick this change to 11.x to help with https://bugs.llvm.org/show_bug.cgi?id=46743? The cache also relies on the dlpi_adds / dlpi_subs fields that are only added in Android R (the upcoming version).

hans added a comment.Aug 19 2020, 7:24 AM

Should we cherry-pick this change to 11.x to help with https://bugs.llvm.org/show_bug.cgi?id=46743? The cache also relies on the dlpi_adds / dlpi_subs fields that are only added in Android R (the upcoming version).

Yes, and also for https://bugs.llvm.org/show_bug.cgi?id=47181

Cherry-picked as 33c13cd8c5772871ec79f0b061e7f7a2fb287383.