[GR-49402] Correct mangling of primitive types in BFD name mangler #7423

adinn · 2023-09-14T15:57:16Z

This PR fixes issue #7421 (q.v.)

The issue is susceptible to a simple fix and a more sophisticated fix as outlined below. The current patch implements the more sophisticated patch in order to minimize symbol footprint

Basic fix

The simplest solution to the problem is to encode primitive type name using embedded symbol names. So, given the two examples cited in the issue

void java.util.Arrays::fill(byte[]*, int, int, byte)
java.lang.String[]* java.lang.String::split(java.lang.String*, int, boolean)

the encodings would be

_ZN16java.util.Arrays4fillEJvP6byte[]ii4byte
_ZN16java.lang.String5splitEJP18java.lang.String[]PS_i7boolean

which successfully decode as desired

$ c++filt _ZN16java.util.Arrays4fillEJvP6byte[]ii4byte
void java.util.Arrays::fill(byte[]*, int, int, byte)
$ c++filt _ZN16java.lang.String5splitEJP18java.lang.String[]PS_i7boolean
java.lang.String[]* java.lang.String::split(java.lang.String*, int, boolean)

Note that the names 4byte, 7boolean and 4char refer to typedefs for primitive types defined in the debug info section so analysis tools which process these names will still be able to resolve them to appropriate machine layouts.

Better fix

The problem with the basic fix is that it significantly enlarges symbol names when signatures include repeat occurrences of the same primitive type:

int jdk.internal.misc.Unsafe::makeInt(byte, byte, byte, byte)
  --> _ZN24jdk.internal.misc.Unsafe7makeIntEJi4byte4byte4byte4byte

$ c++filt _ZN24jdk.internal.misc.Unsafe7makeIntEJi4byte4byte4byte4byte
int jdk.internal.misc.Unsafe::makeInt(byte, byte, byte, byte)

A better solution profits from the use of short names to refer to elements that have already been encoded.

In the above case the encoding of the class name(space) as part of the method name introduces the short name S_ for jdk.internal.misc.Unsafe. Likewise, encoding of the first parameter type introduces short name S0_ for type name byte. The latter binding meant that repeated occurrences of 4byte can be replaced with S0_.

int jdk.internal.misc.Unsafe::makeInt(byte, byte, byte, byte)
  --> _ZN24jdk.internal.misc.Unsafe7makeIntEJi4byteS0_S0_S0_

$ c++filt _ZN24jdk.internal.misc.Unsafe7makeIntEJi4byteS0_S0_S0_
int jdk.internal.misc.Unsafe::makeInt(byte, byte, byte, byte)

Implementing short name substitution only provides a small gain in footprint when encoding primitive symbol names (2 bytes per char or byte parameter, 5 per boolean parameter). However, it can shorten symbols much more significantly when object type names are repeated. For example without short name substitution the following encoding arises for method Arrays::deepEquals:

boolean java.util.Arrays::deepEquals(java.lang.Object[]*, java.lang.Object[]*)
  --> _ZN16java.util.Arrays10deepEqualsEJ7booleanP18java.lang.Object[]P18java.lang.Object[]

$ c++filt _ZN16java.util.Arrays10deepEqualsEJ7booleanP18java.lang.Object[]P18java.lang.Object[]
boolean java.util.Arrays::deepEquals(java.lang.Object[]*, java.lang.Object[]*)

The repeated Object[]* argument can be encoded using a short name as follows:

boolean java.util.Arrays::deepEquals(java.lang.Object[]*, java.lang.Object[]*)
  --> _ZN16java.util.Arrays10deepEqualsEJ7booleanP18java.lang.Object[]S2_

$ c++filt _ZN16java.util.Arrays10deepEqualsEJ7booleanP18java.lang.Object[]S2_
boolean java.util.Arrays::deepEquals(java.lang.Object[]*, java.lang.Object[]*)

providing a much greater saving in footprint (18 bytes).

zakkak · 2023-09-18T09:22:59Z

A better solution profits from the use of short names to refer to elements that have already been encoded.

FWIW, this is documented in https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-compression

zakkak

LGTM, I added a couple of comments/questions that might help improve the PR.

...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java

olpaw · 2023-09-19T14:35:23Z

...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java

+        }
+
+        private boolean substituteName(LookupName name) {
+            int index = bindings.indexOf(name);


Linear search ... are we sure that this does not (ever) explode (computationally)

The lookup cost here will be o(N^2) where N is the number of substitutable elements in the name and signature. N accumulates at most

1 for the package (namespace) prefix
2 for an object return type
2 for an object or (any) array type argument
1 if we have a boolean argument
1 if we have a char argument
1 if we have a byte argument

The count for an object argument/return type can be lowered if

the object type is a foreign type (not passed as a pointer) - subtract 1
the object type repeats the namespace prefix (e.g. java.lang.String) - subtract 1
the object type repeats a previous occurrence - subtract 2

I added a temporary tweak to the patch to count the bindings length for each symbol encoded and tested it by building the image used for the debuginfo test. The worst case I found was this one:

java.util.Collection* sun.security.provider.certpath.DistributionPointFetcher::getCRLs(java.security.cert.X509CRLSelector*, sun.security.x509.X509CertImpl*, sun.security.x509.DistributionPoint*, boolean[]*, boolean, java.security.PublicKey*, java.security.cert.X509Certificate*, java.lang.String*, java.util.List*, java.util.Set*, java.util.Date*, java.lang.String*, java.security.cert.TrustAnchor*)

Encoding it generates 26 bindings. It has a lot of args and the only repeat is the second occurrence of java.lang.String* which appears in the encoding as SG_ i.e. binding 18.

That case is very much an outlier. Normally the number of bindings is quite small. Here is a table summarizing the distribution for the 65667 different encodings generated during generation of one of the test images:

Bindings Count Frequency %ge

1 12721 19.4

2 7482 11.4

3 24745 37.7

4 6049 9.2

5 8164 12.4

6 2509 3.8

7 2578 3.9

8 358 5.4

9 665 1.0

10 108 0.1

11-15 260 0.39

16-20 24 ~0.0

20-25 3 ~0.0

26 1 ~0.0

I don't think this means we must switch to a constant time lookup model such as an EconomicMap but the trade off is not clear. What do you think?

Thanks for the detailed breakdown.

Of course one can think of pathological methods (potentially from generated source code or test suites) that could be much worse than what you found. The question is if it's practically relevant. I would suggest you build a large test image (huge quarkus or springboot native app ... @zakkak might be able to help with that) and check if you can see any real world build time regressions that are distinguishable from noise when building with -g.

I could even use the "temporary tweak to the patch to count the bindings length" to report some more stats, other than the build times. @adinn would you mind sharing that patch, to make sure we are reporting the same thing?

@zakkak Not much to share really. The temporary tweak was simply a call to System.out.format() at the end of BFDMangle::mangle(String, ResolvedJavaType, String, Signature, boolean), printing the resulting mangled name and the length of bindings. The rest of the work was done using shell commands on the resulting output.

@adinn this is a kind reminder

Well it looks like I was looking at a cached version of this when I pinged you :/

Thanks for the reply @adinn , I will try to get some number (hopefully this week)

galderz · 2023-09-21T16:55:27Z

FWIW, this is documented in https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-compression

I looked at @adinn's examples and the documentation and couldn't make sense why S2_ would convert to java.lang.Object[]*. I initially thought S0_ would be enough but not true. I found a better explaination in https://github.com/gchatelet/gcc_cpp_mangling_documentation. That one explains why S0_ is boolean, but I still had doubts why S1_ would not do it. So, here's some further clarification:

$ c++filt "_ZN16java.util.Arrays10deepEqualsEJ7booleanP18java.lang.Object[]S1_"
boolean java.util.Arrays::deepEquals(java.lang.Object[]*, java.lang.Object[])

java.lang.Object[] is a symbol per se:

$ c++filt "_Z18java.lang.Object[]"
java.lang.Object[]

So the fact that is a parameter makes an additional symbol FWIU.

olpaw · 2023-10-11T09:52:41Z

...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java

+         * resulting bindings would be S_ ==> Hello, S0_ ==> Hello::compareTo, S1_ ==> Hello and S2_
+         * ==> Hello* i.e. both S_ and S1_ would demangle to Hello. This situation should never
+         * arise. A name can always be correctly encoded without repeats. In the above example that
+         * woudl be _ZN5Hello9compareToEJiPS_.


Thanks for re-reviewing, Paul. I fixed this and also squashed the style fixes into the main patch and review rework patches.

findbugs fix

style fixes typo

olpaw

Overall LGTM now. Only cosmetic suggestions.

Ready to merge if @zakkak reports that this does not cause performance regression.

...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java

adinn · 2023-10-11T13:13:19Z

@olpaw Remaining recommendations have been followed. Hopefully this will pass the gate and be ready for inclusion.

olpaw · 2023-10-11T14:49:16Z

Running in internal CI ... ⌛

zakkak · 2023-10-16T08:29:29Z

Thanks for the updates @adinn and sorry for not performing the perf runs in time @olpaw, it was a busy week.

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 14, 2023

adinn added native-image native-image-debuginfo redhat-interest labels Sep 14, 2023

adinn requested review from fniephaus, olpaw and zakkak September 15, 2023 13:49

zakkak approved these changes Sep 18, 2023

View reviewed changes

zakkak reviewed Sep 19, 2023

View reviewed changes

...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java Outdated Show resolved Hide resolved

zakkak reviewed Sep 19, 2023

View reviewed changes

...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java Outdated Show resolved Hide resolved

olpaw reviewed Sep 19, 2023

View reviewed changes

...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java Show resolved Hide resolved

olpaw reviewed Sep 19, 2023

View reviewed changes

...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java Outdated Show resolved Hide resolved

olpaw reviewed Sep 19, 2023

View reviewed changes

adinn force-pushed the fix_primitive_arg_mangling branch from 6c11c56 to 7d93199 Compare October 9, 2023 09:16

adinn requested a review from olpaw October 10, 2023 07:55

olpaw reviewed Oct 11, 2023

View reviewed changes

adinn added 4 commits October 11, 2023 11:50

Correct mangling of primitive types in BFD name mangler

867dd65

findbugs fix

Foivos review feedback

56eee31

Further Foivos review feedback

4faa5d3

Paul Woegerer review changes

5f6c589

style fixes typo

adinn force-pushed the fix_primitive_arg_mangling branch from 8e80df1 to 5f6c589 Compare October 11, 2023 10:50

olpaw approved these changes Oct 11, 2023

View reviewed changes

Further changes after Paul Woegerer re-review

afee881

olpaw changed the title ~~Correct mangling of primitive types in BFD name mangler~~ [GR-49402] Correct mangling of primitive types in BFD name mangler Oct 11, 2023

olpaw linked an issue Oct 12, 2023 that may be closed by this pull request

Problem with mangling of primitive types in BFD name mangler #7421

Closed

fniephaus assigned adinn Oct 12, 2023

fniephaus approved these changes Oct 12, 2023

View reviewed changes

graalvmbot merged commit e5766f4 into oracle:master Oct 14, 2023

Bindings Count	Frequency	%ge
1	12721	19.4
2	7482	11.4
3	24745	37.7
4	6049	9.2
5	8164	12.4
6	2509	3.8
7	2578	3.9
8	358	5.4
9	665	1.0
10	108	0.1
11-15	260	0.39
16-20	24	~0.0
20-25	3	~0.0
26	1	~0.0

[GR-49402] Correct mangling of primitive types in BFD name mangler #7423

[GR-49402] Correct mangling of primitive types in BFD name mangler #7423

Uh oh!

Conversation

adinn commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Basic fix

Better fix

Uh oh!

zakkak commented Sep 18, 2023

Uh oh!

zakkak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olpaw Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galderz commented Sep 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olpaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adinn commented Oct 11, 2023

Uh oh!

olpaw commented Oct 11, 2023

Uh oh!

zakkak commented Oct 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

adinn commented Sep 14, 2023 •

edited

Loading

olpaw Sep 19, 2023 •

edited

Loading