Remove handling of outdated 'sr' expressions that interfere with modern encoding #173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looking at the either. It's pretty odd that The mangled version of: |
|
Note the ABI has an issue filed against it about this case: |
|
Presumably the issue filed on the ABI will be resolved by GCC conforming, not by an ABI change. |
|
@fitzgen do you have an opinion on this? |
fitzgen
approved these changes
Apr 5, 2019
tests/tests.rs
Outdated
| @@ -117,7 +117,8 @@ does_not_parse!(close); | |||
|
|
|||
| // Test some potential stack-overflows due to cyclic template parameter references | |||
| does_not_demangle!(_Z1fIT_EvT_); | |||
| does_not_demangle!(_ZN7mozilla6detail12ListenerImplINS_14AbstractThreadEZNS_20MediaEventSourceImplILNS_14ListenerPolicyE0EJNS_13TimedMetadataEEE15ConnectInternalIS2_NS_12MediaDecoderEMS8_FvOS5_EEENS_8EnableIfIXsr8TakeArgsIT1_EE5valueENS_18MediaEventListenerEE4TypeEPT_PT0_SD_EUlS9_E_JS5_EE17ApplyWithArgsImplISL_EENSC_IXsr8TakeArgsISH_EE5valueEvE4TypeERKSH_S9_); | |||
| // This does demangle, but it doesn't demangle the same as llvm-cxxfilt. | |||
| //does_not_demangle!(_ZN7mozilla6detail12ListenerImplINS_14AbstractThreadEZNS_20MediaEventSourceImplILNS_14ListenerPolicyE0EJNS_13TimedMetadataEEE15ConnectInternalIS2_NS_12MediaDecoderEMS8_FvOS5_EEENS_8EnableIfIXsr8TakeArgsIT1_EE5valueENS_18MediaEventListenerEE4TypeEPT_PT0_SD_EUlS9_E_JS5_EE17ApplyWithArgsImplISL_EENSC_IXsr8TakeArgsISH_EE5valueEvE4TypeERKSH_S9_); | |||
There was a problem hiding this comment.
Is it worth still asserting that it demangles in our tests and referencing the follow up issue? That is, if we think it should demangle, then we should assert that, even if it doesn't format exactly what we want yet.
…rn encoding. How we got here: In late 2017 (PR gimli-rs#102) I wrote f6bcf4c. The example symbol for that change was `_ZN4base8internal14CheckedSubImplIlEENSt9enable_ifIXsrSt14numeric_limitsIT_E10is_integerEbE4typeES4_S4_PS4_`. I now believe that symbol is invalid and that its production is either a GNU bug or a nonstandard extension. The latest cxxabi defines `expression` at http://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.expression. It does not have any special casing of `sr` outside of the `unresolved-name` production. These changes were made in 2010 (http://sourcery.mentor.com/archives/cxx-abi-dev/msg02279.html) but GCC and binutils don't seem to have caught up. If we remove our handling of GCC's outdated behavior, then the symbol `_Z6IsNullIiEN1EIXsr1FIT_EE1nEE4typeES1_` from our tests now demangles to `E<F<int>::n>::type IsNull<int>(int)` which is both more sensible and matches llvm-cxxfilt. The symbol `_ZN4base8internal14CheckedSubImplIlEENSt9enable_ifIXsrSt14numeric_limitsIT_E10is_integerEbE4typeES4_S4_PS4_` is no longer valid. The expression (starting with the 'X' after enable_if) needs to be read as `sr <unresolved-qualifier-level>+ E <base-unresolved-name>` Unlike the `type` production which allows (among others a well-known) substitution, `unresolved-qualifier-level` is `simple-id`, so `srSt` must become `sr3std`. The `unresolved-qualifier-level` list must be terminated with an 'E', which must be inserted before '10is_integer'. This also drops std::numeric_limits from the substitution table (it was previously at slot 3), so we need to adjust the substitution indexes at the end. That produces a corrected symbol: `_ZN4base8internal14CheckedSubImplIlEENSt9enable_ifIXsr3std14numeric_limitsIT_EE10is_integerEbE4typeES3_S3_PS3_`, which both llvm-cxxfilt and us agree is demangled to what we were previously demangling it to. roc added `_ZN7mozilla6detail12ListenerImplINS_14AbstractThreadEZNS_20MediaEventSourceImplILNS_14ListenerPolicyE0EJNS_13TimedMetadataEEE15ConnectInternalIS2_NS_12MediaDecoderEMS8_FvOS5_EEENS_8EnableIfIXsr8TakeArgsIT1_EE5valueENS_18MediaEventListenerEE4TypeEPT_PT0_SD_EUlS9_E_JS5_EE17ApplyWithArgsImplISL_EENSC_IXsr8TakeArgsISH_EE5valueEvE4TypeERKSH_S9_` as a symbol that loops infinitely, but it now demangles. It also demangled previously in llvm-cxxfilt. Our demanglings don't match, but I don't propose to solve that today. Finally, we come to the symbol that actually motivated this: `_ZSt3powIdiEN9__gnu_cxx11__promote_2IT_T0_NS0_9__promoteIS2_Xsr3std12__is_integerIS2_EE7__valueEE6__typeENS4_IS3_Xsr3std12__is_integerIS3_EE7__valueEE6__typeEE6__typeES2_S3_`. This is pretty similar to the first symbol, after fixing it up. Here, after the 'X' `expression` production, there is another template argument. The second 'E' (which is required to terminate the `unresolved-qualifier-level` list) gets misinterpreted by the existing code as terminating the containing template production without the next template argument. Parsing then goes off the rails from there. Thus, the correct standards-compliant behavior is incompatible with the outdated output of gcc.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
How we got here: In late 2017 (PR #102) I wrote f6bcf4c. The example symbol for that change was
_ZN4base8internal14CheckedSubImplIlEENSt9enable_ifIXsrSt14numeric_limitsIT_E10is_integerEbE4typeES4_S4_PS4_. I now believe that symbol is invalid and that its production is either a GNU bug or a nonstandard extension.The latest cxxabi defines
expressionat http://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.expression. It does not have any special casing ofsroutside of theunresolved-nameproduction. These changes were made in 2010 (http://sourcery.mentor.com/archives/cxx-abi-dev/msg02279.html) but GCC and binutils don't seem to have caught up.If we remove our handling of GCC's outdated behavior, then the symbol
_Z6IsNullIiEN1EIXsr1FIT_EE1nEE4typeES1_from our tests now demangles toE<F<int>::n>::type IsNull<int>(int)which is both more sensible and matches llvm-cxxfilt.The symbol
_ZN4base8internal14CheckedSubImplIlEENSt9enable_ifIXsrSt14numeric_limitsIT_E10is_integerEbE4typeES4_S4_PS4_is no longer valid. The expression (starting with the 'X' after enable_if) needs to be read assr <unresolved-qualifier-level>+ E <base-unresolved-name>Unlike the
typeproduction which allows (among others a well-known) substitution,unresolved-qualifier-levelissimple-id, sosrStmust becomesr3std. Theunresolved-qualifier-levellist must be terminated with an 'E', which must be inserted before '10is_integer'. This also drops std::numeric_limits from the substitution table (it was previously at slot 3), so we need to adjust the substitution indexes at the end. That produces a corrected symbol:_ZN4base8internal14CheckedSubImplIlEENSt9enable_ifIXsr3std14numeric_limitsIT_EE10is_integerEbE4typeES3_S3_PS3_, which both llvm-cxxfilt and us agree is demangled to what we were previously demangling it to.roc added
_ZN7mozilla6detail12ListenerImplINS_14AbstractThreadEZNS_20MediaEventSourceImplILNS_14ListenerPolicyE0EJNS_13TimedMetadataEEE15ConnectInternalIS2_NS_12MediaDecoderEMS8_FvOS5_EEENS_8EnableIfIXsr8TakeArgsIT1_EE5valueENS_18MediaEventListenerEE4TypeEPT_PT0_SD_EUlS9_E_JS5_EE17ApplyWithArgsImplISL_EENSC_IXsr8TakeArgsISH_EE5valueEvE4TypeERKSH_S9_as a symbol that loops infinitely, but it now demangles. It also demangled previously in llvm-cxxfilt. Our demanglings don't match, but I don't propose to solve that today.Finally, we come to the symbol that actually motivated this:
_ZSt3powIdiEN9__gnu_cxx11__promote_2IT_T0_NS0_9__promoteIS2_Xsr3std12__is_integerIS2_EE7__valueEE6__typeENS4_IS3_Xsr3std12__is_integerIS3_EE7__valueEE6__typeEE6__typeES2_S3_. This is pretty similar to the first symbol, after fixing it up. Here, after the 'X'expressionproduction, there is another template argument. The second 'E' (which is required to terminate theunresolved-qualifier-levellist) gets misinterpreted by the existing code as terminating the containing template production without the next template argument. Parsing then goes off the rails from there. Thus, the correct standards-compliant behavior is incompatible with the outdated output of gcc.