HomePhabricator

[libc++] NFCI: Remove _LIBCPP_EXTERN_TEMPLATE2

Authored by ldionne on Oct 2 2020, 11:29 AM.

Description

[libc++] NFCI: Remove _LIBCPP_EXTERN_TEMPLATE2

This seems to have been added a long time ago as a temporary help
for debugging some <regex> issue, but it's really the same as
_LIBCPP_EXTERN_TEMPLATE.

Details

Committed
ldionneOct 2 2020, 11:31 AM
Parents
rG84feca6a84d9: [MemCpyOpt] Add tests from D40802 (NFC)
Branches
Unknown
Tags
Unknown

Event Timeline

rnk added a subscriber: rnk.Jan 13 2021, 3:36 PM

This actually caused https://crbug.com/1166386. The explanation is that defining _LIBCPP_DEBUG causes _LIBCPP_EXTERN_TEMPLATE to be defined to nothing, which disables the extern declarations. Chrome uses _LIBCPP_DEBUG in debug builds. IIUC, locale needs these extern template declarations, because locale uses static data members with hidden visibility (need to confirm). If you remove the extern template declarations, then duplicate static data members may be emitted into the user's DSO, and when they use locale facets, they will be registered with a new id. I need to dig more to confirm, but this is what I'm going on.

ldionne added a subscriber: EricWF.Jan 14 2021, 1:05 PM

This actually caused https://crbug.com/1166386. The explanation is that defining _LIBCPP_DEBUG causes _LIBCPP_EXTERN_TEMPLATE to be defined to nothing, which disables the extern declarations. Chrome uses _LIBCPP_DEBUG in debug builds. IIUC, locale needs these extern template declarations, because locale uses static data members with hidden visibility (need to confirm). If you remove the extern template declarations, then duplicate static data members may be emitted into the user's DSO, and when they use locale facets, they will be registered with a new id. I need to dig more to confirm, but this is what I'm going on.

Hmm, interesting. I guess the debug mode is disabling the extern template declarations because there are debug checks in those functions, and we want the functions to actually perform the checks, not use the version compiled in the library (which don't have the checks since the library might not have been built with the debug mode enabled). There are several issues with our debug mode implementation. I see a few options:

  1. Require that the dylib be built with debug mode enabled in order to turn on the debug mode (that way, we can use the debug-mode-enabled functions built into the dylib)
  2. Change the debug mode to not require any dylib support (that way, we can use the functions built into the dylib, which don't care about whether the debug mode was enabled or not when they were built)

@EricWF Had plans for doing (2) using UBSan, but I don't know what the status of that work is.

By the way, I think it is actually the following commit that broke you:

commit 31e820378b8ae4d81e9d206a7dae64ccf4b4c97f
Author: Louis Dionne <ldionne@apple.com>
Date:   Fri Oct 2 15:02:52 2020 -0400

    [libc++] NFCI: Simplify macro definitions for the debug mode
    
    The debug mode always had three possibilities:
    - _LIBCPP_DEBUG is undefined => no assertions
    - _LIBCPP_DEBUG == 0         => some assertions
    - _LIBCPP_DEBUG == 1         => some assertions + iterator checks
    
    This was documented that way, however the code did not make this clear
    at all. The discrepancy between _LIBCPP_DEBUG and _LIBCPP_DEBUG_LEVEL
    was especially confusing. I reworked how the various macros are defined
    without changing anything else to make the code clearer.

Before that commit, under debug mode, we'd disable _LIBCPP_EXTERN_TEMPLATE, but not _LIBCPP_EXTERN_TEMPLATE2. After the commit, we disable both (which is your issue).

rnk added a comment.Jan 14 2021, 5:22 PM

Before that commit, under debug mode, we'd disable _LIBCPP_EXTERN_TEMPLATE, but not _LIBCPP_EXTERN_TEMPLATE2. After the commit, we disable both (which is your issue).

Well, the bisect turned up this commit, so I'm pretty sure it is this one. Maybe what you mean is that, reverting this commit isn't sufficient in light of the following commit. Reverting this commit alone will just add a duplicate second macro (_LIBCPP_EXTERN_TEMPLATE2) controlled by _LIBCPP_DISABLE_EXTERN_TEMPLATE, which is now controlled by _LIBCPP_DEBUG. They sort of work together to create the issue.

Anyway, it's all the same, thanks for the fix.