Page MenuHomePhabricator

Promote nameless lambda used by dl_iterate_phdr to named function to clean up control flow inside findUnwindSections. Also, expose the data structureto allow use by a future replacment function.
ClosedPublic

Authored by saugustine on Mar 2 2020, 2:16 PM.

Details

Summary

[Refactor] Promote nameless lambda to fully named function, allowing easy replacement in following patch.

Diff Detail

Event Timeline

saugustine created this revision.Mar 2 2020, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2020, 2:16 PM
saugustine retitled this revision from Promote nameless lambda used by dl_iterate_phdr to named function to clean up control flow inside findUnwindSections. Also, expose the data structure to allow use by a future replacment function. to Promote nameless lambda used by dl_iterate_phdr to named function to clean up control flow inside findUnwindSections. Also, expose the data structureto allow use by a future replacment function..Mar 2 2020, 2:20 PM
echristo accepted this revision.Mar 2 2020, 2:58 PM
echristo added a subscriber: echristo.

LGTM.

This revision is now accepted and ready to land.Mar 2 2020, 2:58 PM
MaskRay accepted this revision.Mar 2 2020, 3:05 PM

So findUnwindSectionByPhdr was the lambda. Makes sense.

This revision was automatically updated to reflect the committed changes.

This broke building for windows, and I would be pretty sure it also broke things for apple platforms as well (as _LIBUNWIND_SUPPORT_DWARF_UNWIND is defined on both of them, at least on certain architectures).

There's a long chain of ifdefs in LocalAddressSpace::findUnwindSections, like this:

#ifdef __APPLE__
#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_LIBUNWIND_IS_BAREMETAL)
#elif defined(_LIBUNWIND_ARM_EHABI) && defined(_LIBUNWIND_IS_BAREMETAL)
#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_WIN32)
#elif defined(_LIBUNWIND_SUPPORT_SEH_UNWIND) && defined(_WIN32)
#elif defined(_LIBUNWIND_ARM_EHABI) && defined(__BIONIC__)
#elif defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
// Current ELF code being refactored
#endif

Now after this change, you've placed a separate #if defined(_LIBUNWIND_ARM_EHABI) || defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) standalone above all of it - while _LIBUNWIND_SUPPORT_DWARF_UNWIND is defined in a lot of other cases as well (both on apple targets and for some architectures on win32), that are handled by other more specialised OS specific ifdefs in LocalAddressSpace::findUnwindSections.

Not sure what the cleanest solution would be though. The initial thought would be to add && !defined(__APPLE__) && !defined(_WIN32) && !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(__BIONIC__) to the ifdef surrounding findUnwindSectionByPhdr, but that doesn't quite exactly replicate the effect of the long series of ifdefs either. The safest (but less neat) solution would be to add the exact same series of ifdefs around findUnwindSectionByPhdr, with empty ifdef bodies for all the other cases.

miyuki added a subscriber: miyuki.Mar 4 2020, 4:02 AM

This also broke the build for baremetal configurations: before the patch the lambda was ifdef-ed out for cases like

#elif defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND) && defined(_LIBUNWIND_IS_BAREMETAL)
...
#elif defined(_LIBUNWIND_ARM_EHABI) && defined(_LIBUNWIND_IS_BAREMETAL)
...

After the patch the code in the findUnwindSectionByPhdr function causes compile-time errors because of undefined symbols (e.g. ElfW) and accesses to the incomplete type dl_phdr_info. As the previous comment suggested, findUnwindSectionByPhdr should be ifdef-ed out when _LIBUNWIND_IS_BAREMETAL is defined.

Sorry for the trouble. Trying again in D75637. There really doesn't seem to be a clean way of #ifdeffing this.