[GR-41047] use C++ symbol mangling on Linux to make tools happy #4950
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
adinn
force-pushed
the
mangling_support
branch
6 times, most recently
from
adbc2ab to
47e81a3
Compare
fniephaus
changed the title
|
Unfortunately, AOT-compiling Graal.js fails with this: Any idea what could be going on here, @adinn? |
|
@fniephaus Hi. I have an idea what might going on here but I may need to debug the problem to be sure. The error means that the map from ResolvedJavaMethod to encoded name is not 1-1. Clearly this cannot the case for
Can you provide me with instructions on how to run the gate test in a debugger? If that is not possible can you provide a reproducer I can run that recreates the problem? |
|
Ok, scratch that last comment. I see now that the problem is upstream of method This relates to the patch Paul added to I can easily inherit Paul's disambiguation fix by making I fear this fix may not be the end of the story as the resulting changes to the demangled name may cause problems for the tools. For non-bootstrap classes or methods which require a disambiguation suffix the demangled name will fail to be an exact match for both the name encoded via DWARF and the Java source name. The disparity might not confuse users very much as far as the ability to identify which method is at play is concerned (although it would certainly not be completely transparent). However, I am not clear what problems these deviations might cause perf and/or valgrind to suffer: firstly, during reporting, when trying to connect profile data with the relevant sources and disassembliesl; and secondly, on the command line, when a user needs to specify the method(s) for which reports of profile data are desired. I don't think the method name ambiguity will constitute a significant problem. This type of ambiguity only in rare cases. However, I think the loader issue is something we need to resolve. One thing that strikes me is that we don't actually need classloader prefixes for the system or module loader. Names of classes in all 3 initial loaders should not duplicate each other (the delegation rules ought to ensure that is the case). If so then that would mean a lot less opportunity for tools to be stymied by the name change. In the case where the class is in a user loader not on the system path a solution to the DWARF name mismatch might be to embed the class loader name into the DWARF info. We could just prepend it to the class info Any thoughts? |
Correct. The classloader must play it's role in uniqueness of the returned String.
This can be implemented as an optimization in |
|
Also I see internal aarch64 gates failing with these changes with e.g. |
|
New version factors out unique short name generation into a provider and configures a BFD-compatible provider when generating debug info on Linux. |
adinn
force-pushed
the
mangling_support
branch
2 times, most recently
from
819e1cb to
ca079b5
Compare
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/UniqueShortNameProvider.java
Outdated
Show resolved
Hide resolved
| public abstract class UniqueShortNameProvider { | ||
| private static UniqueShortNameProvider provider = null; | ||
|
|
||
| public static synchronized UniqueShortNameProvider provider() { |
There was a problem hiding this comment.
The convention we usually use here is to name such methods singleton().
It's not a good idea to make this method smart.
I.e. do not make the method synchronized to create a fallback here on first access but instead make sure that ImageSingletons.lookup(UniqueShortNameProvider.class) always returns a valid provider so that the implementation is just return ImageSingletons.lookup(UniqueShortNameProvider.class); and nothing else.
In simple cases you can use annotation @AutomaticallyRegisteredImageSingleton or if you need more flexibility use an internal feature (with @AutomaticallyRegisteredFeature) that uses afterRegistration to set up the UniqueShortNameProvider.
There was a problem hiding this comment.
I have changed this to use name singleton and simply do an unsynchronized lookup. The default provider is now factored out of the interface and installed as an AutomaticallyRegisteredImageSingleton. It uses an associated BooleanSupplier to enable registration when we have no debug info enabled or are not on Linux.
I have retained the DebugInfoFeature with an afterRegistration callback in order to ensure that the BFD provider gets registered when we do have debug info enabled and are on Linux.
There was a problem hiding this comment.
Sounds good.
Please make sure you use the same BooleanSupplier in both so that it's easy to find the belonging parts in an IDE.
There was a problem hiding this comment.
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/UniqueShortNameProvider.java
Outdated
Show resolved
Hide resolved
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/UniqueShortNameProvider.java
Outdated
Show resolved
Hide resolved
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/UniqueShortNameProvider.java
Outdated
Show resolved
Hide resolved
|
@olpaw I have just pushed commits for the rework and for the DWARF changes to embed child DIEs for class compile units with a named loader in a wrapper namespace DIE. n.b. I had to update the API for That's needed because Assuming there are nor style or format errors this is ready for re-review. |
adinn
force-pushed
the
mangling_support
branch
4 times, most recently
from
d2a7814 to
74de69e
Compare
|
@olpaw The latest push is complete, has no style errors and passes the gate tests apart from two sulong errors unrelated to the patch. gdb, perf and valgrind all successfully recognize the DWARF info and demangle the ELF symbols correctly. I also tested with your test program that has two versions of the same class. I had to tweak the pom to enable javac debug info and had build source jars for Main, Module1 and Module2. I also had to include the source jars for the Module classes in the source search path (the Main sources are found because they sit next to the Main jar). gdb identifies both versions of the method located under a namespace derived from the relevant classloader id as shown in the session listing that follows: Notice that the two breakpoints are reported at different lines. Of course, the source cache only contains one version of the file -- the one found in the first source jar on the source path. So, the file view when at the second break displays the 'wrong' file, locating the current line in the comment. However, that is only to be expected. |
|
Just to be 100% sure, I did the following experimental change: diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java
index 7c911ce..bcacbce 100644
--- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java
+++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java
@@ -363,7 +363,7 @@ class NativeImageBFDNameProvider implements UniqueShortNameProvider {
public String mangle(String loaderName, ResolvedJavaType declaringClass, String memberName, Signature methodSignature, boolean isConstructor) {
String fqn = declaringClass.toJavaName();
String selector = memberName;
- if (isConstructor) {
+ if (isConstructor && false) {
assert methodSignature != null;
assert selector.equals("<init>");
int index = fqn.lastIndexOf('.');which fixes the issue. In other words for constructors |
|
@matneu this PR needs a security review because it modified the symbol names we generate in native-images when the user builds with debug-info generation ( |
|
@olpaw I just pushed a fix that includes the guarantee for uniqueness of the fixed name and modifies the generator to retain any suffix on the constructor name. |
|
@adinn rerunning the CI gates right now ... |
|
@adinn looks like all gates are passing now. There were two failing gates but I suspect them to be transients that I'm rerunning now. |
|
@adinn as usual, please fold style-fix commits into their dominating commits and make all commit messages start with an upper-case letter. After that I'm going to merge this. |
adinn
force-pushed
the
mangling_support
branch
from
95c0e03 to
6268d60
Compare
|
Ok, all squashed and ready to ship :-) |
|
Ah, except we have a conflict to resolve. |
Revert language attribute and fix docs/style issues
…inux DWARF debug info Style fixes
adinn
force-pushed
the
mangling_support
branch
from
6268d60 to
3d15b72
Compare
|
@olpaw Ok, that was easy to fix. I had to ensure reflection stub names (based on a digest) start with alpha not alphanum since the C++ encoding uses a length count prefix and a leading digit blends into the count. My addition of an "invoke_" prefix to the name clashed with an independent change to the same line. I don't think this is going to break anything. Do you need to run the gate test again? |
Yes. On it. If all goes well should be on master by the end of this week. |
This patch encodes local symbols on Linux using a C++-like bfd compatible encoding of the Java method name and signature. This allows both gdb and Linux tools to demangle method names in most circumstances, including in assembly code listings. If works best best used in combination with option
-H:-DeleteLocalSymbolsbut still improved behaviour for tools without passing that option.Note that for gdb to display demangled method names after this patch is enabled it is necessary to execute command
(gdb) set print asm-demangle.