New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/link: option to omit DWARF vendor attributes #25297

Open
gwik opened this Issue May 8, 2018 · 20 comments

Comments

Projects
None yet
8 participants
@gwik
Contributor

gwik commented May 8, 2018

Hi,

I'm trying to make the crashlytics symbol extractor to accept to parse go libraries generated with gomobile.
However it crashes because go use "vendor" or "user" DWARF attributes (e.g: DW_AT_go_kind = 0x2900).
Usage is perfectly standard, yet it crashes every time it encounters such attribute. I guess that, while standard, this is uncommon for "native" libraries. I suppose they are written in C or C++ most of the time and don't include vendor attributes.

My first approach was to try to strip the attributes from the binary. However, this is fairly involved and probably error prone, the DWARF binary encoding doesn't allow much flexibility. It would require to entirely rewrite many DWARF sections.

I figured that making Go not include those attributes in the first place was straightforward and not intrusive since there was already a single place that filters out the attributes before they are written out:

// only the ones actually listed in the Abbrev will be written out.

Thanks to the amazing work of @eliasnaur lately, this is the last piece that prevent go to fully support the mobile debugging and tooling ecosystem. I hope you will consider adding the option to go tool link.
CL follows.

@gopherbot

This comment has been minimized.

gopherbot commented May 8, 2018

Change https://golang.org/cl/112215 mentions this issue: cmd/link: add option to omit go DWARF user attributes

@eliasnaur

This comment has been minimized.

Contributor

eliasnaur commented May 8, 2018

A new flag is a heavy handed solution. Have you reported the issue to Crashlytics? Is the problem for both iOS and Android or only one? What are the vendor DWARF attributes for? Perhaps they can be filtered unconditionally for the problematic platforms, at least until Crashlytics catches up?

@gwik

This comment has been minimized.

Contributor

gwik commented May 8, 2018

Yes, I reported the issue to Crashlytics, I'm waiting for a reply. The issue is on Android only, the crashlytics gradle plugin embeds a ELF/DWARF parser and generate "cSym" file which has the same purpose as the dSym on darwin.

The vendor attributes aims at adding higher level go concepts to the debug information. They can be used by debuggers that are go aware, I suppose. Although it doesn't feel like they are required, DWARF already have a lot of different concepts and was designed for representing any language.

Filtering on the target platform instead of a flag is a good idea, it propably would be a much less binding solution for go.

@steeve

This comment has been minimized.

Contributor

steeve commented May 9, 2018

Although we've only seen the issue with Crashlytics on Android (and they have been informed), one could make the argument that some DWARF parsers could choke on Go metadata, in which case this flag could remedy the situation. Although that might be far fetched.

It is true that this could be enabled by default when GOOS=android, where Crashlytics is likely to be used.

@eliasnaur

This comment has been minimized.

Contributor

eliasnaur commented May 9, 2018

I'm still not convinced of a new flag; crashlytics is a client side program and as soon as they release a fix, the problem is gone for all their users. On the other hand, we'll have to support a new flag indefinitely. Even if a flag were the way forward, I'm not sure you'll get it in Go 1.11 anyway, because of the freeze.

Other than waiting for a response for Crashlytics, I think the best bet is @gwik's stripping of custom attributes. How far did that get? Can you "zero out" the offending attributes, or make them more standard instead of stripping them so you avoid more invasive changes to the DWARF structure?

I'm assuming the Crashlytics parser isn't open source. If it is, one could put in a hack to that instead.

@steeve

This comment has been minimized.

Contributor

steeve commented May 9, 2018

I think there are two issues there:

  1. wether link should dump Go attributes or not
  2. wether that behaviour should be exposed externally in the command line

From what I understand, you are mainly concerned with 2, right? Because 1 could be restricted to GOOS=android.

Ultimately it is true that the burden lies with Crashlytics (or their parser). But since Crashlytics is Google now, I'm not sure a timely fix will be happening.

@gwik

This comment has been minimized.

Contributor

gwik commented May 9, 2018

Can you "zero out" the offending attributes, or make them more standard instead of stripping them so you avoid more invasive changes to the DWARF structure?

That's what I hoped, but I don't think this is possible. Zero attribute marks the end of the attributes and the end of the entry. Although the spec tells that null entries are valid, they mark the end of siblings in the tree and crashlytics parser use a stack to build up its tree representation and unconditionally pops when it sees a null entry.

I also tried to replace with some unused attributes (for crashlytics) that would match the value class but vendor attributes end up being two bytes and "standard" attributes are one byte after variable size encoding (LEB128).

Rewriting the .debug_abbrev and .debug_info section seems inevitable, but then you have to rewrite .debug_aranges, .debug_pubnames, .debug_loc because they reference offsets in the .debug_info table. You end up rewrite everything.

dwarftablerel

@gwik

This comment has been minimized.

Contributor

gwik commented May 9, 2018

I'm assuming the Crashlytics parser isn't open source. If it is, one could put in a hack to that instead.

Unfortunately it's not. Looking at decompiled code is straightforward but you can't modify it.

@eliasnaur

This comment has been minimized.

Contributor

eliasnaur commented May 9, 2018

Pinging @aclements (the owner of debug/dwarf) for good ideas.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented May 9, 2018

I don't know if it will help, but I think we should give Crashlytics a few days to respond. I've also pinged them internally.

@thanm

This comment has been minimized.

Member

thanm commented May 9, 2018

I agree with Ian, seems like it would be better to fix the DWARF consumer.
Crashing on all vendor-specific attributes seems fairly unfriendly; they are not really all that uncommon.

@gwik

This comment has been minimized.

Contributor

gwik commented May 9, 2018

I agree with Ian, seems like it would be better to fix the DWARF consumer.

Off course. Patching go was the easiest way for me to move forward and testing that it would work without the attributes. I perfectly understand that adding the flag is heavy-handed.

Thank you @ianlancetaylor for pinging them, here the answer I got from support (maybe the people you ping didn't communicate with the person who answered me):

Thanks for reaching out on this Antonin, unfortunately, we don't support Go, so the team won't be able to dig into this at the moment. If that changes, I'll let you know!

I will insist and explain them this is not related to Go, but it doesn't start well.

@TKBurner

This comment has been minimized.

TKBurner commented May 10, 2018

Thanks. I'm from the Crashlytics team and it's really interesting to hear what you are up to with Go! Right now it's not on our roadmap to add support for the build process offered by Gomobile.

I really am interested how these crash reports symbolicate if you follow manual symbolication: https://stackoverflow.com/questions/3832900/how-to-manually-symbolicate-ios-crash-to-view-crash-logs

This is generally how we begin to look into these sorts of problems when we consider if we are able to take on new work.

Thanks all and always feel free to reach out 👍

@gwik

This comment has been minimized.

Contributor

gwik commented May 10, 2018

@TKBurner thanks for reaching out here. The issue we are talking about is not related to go nor gomobile. We are talking about how the crashlytics gradle android plugin fails when any native library (NDK) contains DWARF standard user attributes like go ones does.
I’ve run a debugger and looked at the decompiled java code where it raises an exception and the fix is probably a few “if” statements so it can just ignore the user attributes and complete the symbolication process.
Again, this is not a go issue, it is just about fixing the support for DWARF symbol extraction that you already support. All you need to do is to skip the user attribute range from 0x2000 to 0x3fff.

@TKBurner

This comment has been minimized.

TKBurner commented May 10, 2018

@steeve

This comment has been minimized.

Contributor

steeve commented May 10, 2018

Thanks Todd, we already have a thread going on with Michael. We'll let him know.

@eliasnaur

This comment has been minimized.

Contributor

eliasnaur commented Nov 29, 2018

What's the status of this?

@steeve

This comment has been minimized.

Contributor

steeve commented Nov 29, 2018

We sent a patch to the Crashlytics team to add support for the Go DWARF types, which we did thanks to a Java decompiler (fernflower, used by Android Studio).

They have acknowledged the patch, but I'm not sure it was released/mainlined (don't see anything related to that in the Changelog at https://docs.fabric.io/android/changelog.html#september-27-2018).
@TKBurner should know more hopefully.

In the mean time, we recompiled the Crashlytics gradle plugin with the patch. It works well.

This patch basically adds the Go DWARF attributes to the hashmap responsible for holding them.

--- DWAttribute.java.orig	2018-08-23 20:51:48.000000000 +0200
+++ DWAttribute.java	2018-08-23 20:51:48.000000000 +0200
@@ -251,7 +251,12 @@
    APPLE_MAJOR_RUNTIME_VERS(16357, "APPLE_major_runtime_vers"),
    APPLE_RUNTIME_CLASS(16358, "APPLE_runtime_class"),
    APPLE_OMIT_FRAME_PTR(16359, "APPLE_omit_frame_ptr"),
-   HI_USER(16383, "hi_user");
+   HI_USER(16383, "hi_user"),
+   GO_KIND(0x2900, "go_kind"),
+   GO_KEY(0x2901, "go_key"),
+   GO_ELEM(0x2902, "go_elem"),
+   GO_EMBEDDED_FIELD(0x2903, "go_embedded_field"),
+   GO_RUNTIME_TYPE(0x2904, "go_runtime_type");
 
    private static final String PREFIX = "DW_AT_";
    private static final Map LOOKUP;

I can attach the final jar is people are interested, but I'd rather have it mainlined.

Finally, I think we can close that issue, since we've changed course.

@aclements

This comment has been minimized.

Member

aclements commented Nov 29, 2018

@steeve

This comment has been minimized.

Contributor

steeve commented Nov 29, 2018

@aclements you nailed it. Calling javac on this single file was easier.

Here is our Makefile (minus our internal stuff), if people are tempted:

ARTIFACT_GROUP_ID := io.fabric.tools
ARTIFACT_NAME := gradle
ARTIFACT_VERSION := 1.25.4
MAVEN_REPOSITORY := https://maven.fabric.io/public
ARTIFACT_URL_PREFIX := $(MAVEN_REPOSITORY)/$(subst .,/,$(ARTIFACT_GROUP_ID))/$(ARTIFACT_NAME)/$(ARTIFACT_VERSION)
JAR_FILE := $(ARTIFACT_NAME)-$(ARTIFACT_VERSION).jar
POM_FILE := $(ARTIFACT_NAME)-$(ARTIFACT_VERSION).pom
PATCHED_ARTIFACT_VERSION := $(ARTIFACT_VERSION).zenly
PATCHED_JAR_FILE := $(ARTIFACT_NAME)-$(PATCHED_ARTIFACT_VERSION).jar
PATCHED_POM_FILE := $(ARTIFACT_NAME)-$(PATCHED_ARTIFACT_VERSION).pom
FERNFLOWER_VERSION := 2.5.0.Final
FERNFLOWER := fernflower.jar

all: deploy clean

$(FERNFLOWER):
	curl -L -o $(@) http://central.maven.org/maven2/org/jboss/windup/decompiler/fernflower/fernflower/$(FERNFLOWER_VERSION)/fernflower-$(FERNFLOWER_VERSION).jar

$(JAR_FILE):
	curl -L -o $(JAR_FILE) $(ARTIFACT_URL_PREFIX)/$(JAR_FILE)

$(POM_FILE):
	curl -L -o $(POM_FILE) $(ARTIFACT_URL_PREFIX)/$(POM_FILE)

$(PATCHED_JAR_FILE): $(JAR_FILE) $(FERNFLOWER)
	unzip -o $(JAR_FILE) 'com/crashlytics/tools/utils/dwarf/DWAttribute.class'
	java -cp $(FERNFLOWER) org.jetbrains.java.decompiler.main.decompiler.ConsoleDecompiler \
		com/crashlytics/tools/utils/dwarf/DWAttribute.class .
	patch -p0 < DWAttribute_golang_dwarf.patch
	javac -d . DWAttribute.java
	rm -f DWAttribute.java
	cp $(JAR_FILE) $(PATCHED_JAR_FILE)
	jar -uf $(PATCHED_JAR_FILE) com/

$(PATCHED_POM_FILE): $(POM_FILE)
	sed 's/>$(ARTIFACT_VERSION)</>$(PATCHED_ARTIFACT_VERSION)</g' < $(POM_FILE) > $(PATCHED_POM_FILE)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment