Subzero: Deal with substitutions in the primitive remangler.

Subzero: Deal with substitutions in the primitive remangler.

https://refspecs.linuxbase.org/cxxabi-1.75.html#mangling-compression
describes the mechanism for compressing mangled strings by using substitutions of the form S[0-9A-Z]*_ to represent repeated components.

When the prefix is handled as wrapping inside a namespace, the base-36 substitution numbers all have to be incremented.

This is implemented in a very simple way by scanning the string only for instances of the substitution pattern.

Unfortunately, false matches are possible because the S[0-9A-Z]*_ pattern can be a substring of the type name, or can span other components of the mangled name.  Getting this completely right would essentially require a full demangling parser - see the ~4000 lines of code in cxa_demangle.cpp and ItaniumMangle.cpp.

Since this is just for testing, any false matches will likely cause a linking error and the test can be rewritten to avoid false matches.

BUG= none
R=jvoung@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=217dc08

[jvoung (off chromium), 2014-07-11 20:40:09]

LGTM

https://codereview.chromium.org/385273002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/385273002/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:148: for (Last = OldPos + 1; Last < OldLen && Valid;
++Last) {
There's an explicit break; when Valid is set to false, so could probably leave
this && Valid out?

It also seems like "Found" is the only variable that's checked after the break,
so the "Valid" variable can be removed too?

https://codereview.chromium.org/385273002/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:153: } else if (std::isdigit(Ch) || std::isupper(Ch)) {
Should this explicitly pass as parameter the "C" locale, to make sure this is
just A-Z?

https://codereview.chromium.org/385273002/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:170: // Replace N 'Z' characters with N+1 '0'
characters.
This case also handles the "S_" ?

https://codereview.chromium.org/385273002/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/mangle.ll (right):

https://codereview.chromium.org/385273002/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/mangle.ll:107: ;   S_ ==> S0_
It's like "0" is the new "1", but I guess it's also 0 once it's past single
digits.

[Jim Stichnoth, 2014-07-11 21:05:12]

https://codereview.chromium.org/385273002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/385273002/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:148: for (Last = OldPos + 1; Last < OldLen && Valid;
++Last) {
On 2014/07/11 20:40:08, jvoung wrote:
> There's an explicit break; when Valid is set to false, so could probably leave
> this && Valid out?
> 
> It also seems like "Found" is the only variable that's checked after the
break,
> so the "Valid" variable can be removed too?

Done.  BTW, it would also be possible to remove the Found variable by moving the
if(Found){...} code inside the if(Ch=='_'){...} block, but I think it's more
readable the current way.

https://codereview.chromium.org/385273002/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:153: } else if (std::isdigit(Ch) || std::isupper(Ch)) {
On 2014/07/11 20:40:08, jvoung wrote:
> Should this explicitly pass as parameter the "C" locale, to make sure this is
> just A-Z?

Done.

https://codereview.chromium.org/385273002/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:170: // Replace N 'Z' characters with N+1 '0'
characters.
On 2014/07/11 20:40:08, jvoung wrote:
> This case also handles the "S_" ?

Yes.  Added a comment to clarify.

https://codereview.chromium.org/385273002/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/mangle.ll (right):

https://codereview.chromium.org/385273002/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/mangle.ll:107: ;   S_ ==> S0_
On 2014/07/11 20:40:08, jvoung wrote:
> It's like "0" is the new "1", but I guess it's also 0 once it's past single
> digits.

Yeah, S_ is kind of like S<num>_ where num=-1.

[Jim Stichnoth, 2014-07-11 21:06:59]

Committed patchset #3 manually as r217dc08 (presubmit successful).

[wala, 2014-07-11 21:21:56]

https://codereview.chromium.org/385273002/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/mangle.ll (right):

https://codereview.chromium.org/385273002/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/mangle.ll:108: ;   String length increase, e.g. SZZZ_
==> S0000_
The successor of ZZZ in base 36 is 1000. Why 0000 here?

CXXNameMangler::mangleSeqID seems to use straightforward base 36 conversion
(http://clang.llvm.org/doxygen/ItaniumMangle_8cpp_source.html), so it would
represent the successor of ZZZ as 1000.

[Jim Stichnoth, 2014-07-11 22:04:50]

On 2014/07/11 21:21:56, wala wrote:
>
https://codereview.chromium.org/385273002/diff/40001/tests_lit/llvm2ice_tests...
> File tests_lit/llvm2ice_tests/mangle.ll (right):
> 
>
https://codereview.chromium.org/385273002/diff/40001/tests_lit/llvm2ice_tests...
> tests_lit/llvm2ice_tests/mangle.ll:108: ;   String length increase, e.g. SZZZ_
> ==> S0000_
> The successor of ZZZ in base 36 is 1000. Why 0000 here?
> 
> CXXNameMangler::mangleSeqID seems to use straightforward base 36 conversion
> (http://clang.llvm.org/doxygen/ItaniumMangle_8cpp_source.html), so it would
> represent the successor of ZZZ as 1000.

Oops, you're right, I misread the spec.  I'll fix this in a new CL.