Skip to content
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

Print custom section contents if printable #2326

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

RReverser
Copy link
Member

This helps with debugging human-readable sections like sourceMappingURL.

@RReverser
Copy link
Member Author

P.S. Waiting for CI as I'm not sure I updated all the necessary tests (had some spurious failures on Windows).

@RReverser RReverser force-pushed the print-custom-sections branch 2 times, most recently from 67b5a8f to 04e2ca2 Compare September 3, 2019 09:33
@RReverser
Copy link
Member Author

Should be good to go now.

@dcodeIO
Copy link
Contributor

dcodeIO commented Sep 3, 2019

Makes me wonder why custom sections aren't part of text format in the first place. Considering that there's (data ...), which is unreadable most of the time, having something like (section ...) doesn't seem too far off and would allow reassembling a module as a whole without losing this information. Does anybody know if there's a reason for that?

@RReverser
Copy link
Member Author

@dcodeIO I don't know the reason, but I imagine it would be more helpful to look through WebAssembly spec discussions rather than here.

break;
}
}
if (isPrintable) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The majority of custom sections are going to have name elements that include length fields that aren't really printable, so this won't generalize far beyond the sourceMappingURL section. Since many custom sections are tool conventions, though, I wonder if it would make sense to add code here to print each of them with knowledge of their layout and semantics. If that seems useful, perhaps this PR can just do the printing if the name of the section is "sourceMappingURL" instead of inferring if the contents are printable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourceMappingURL was not the initial goal of this PR. It was introduced for testing in #2327, and sourceMappingURL just also happened to be printable when this code was added.

We could hardcode both names, but given this "lucky accident", I'd say let's ship the generic code as-is - 1) it's pretty low-cost since binary sections would quickly bail out, and 2) --print is used only for debugging anyway where such minor slowness wouldn't be noticeable and 3) it will make it work out of the box with other potential tooling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I'm not concerned with performance; I'm concerned that we could write different code that would be more useful. I think properly printing custom sections would be very useful, but I see only limited value in a solution that bails out for most sections with no clear upgrade path.

I may be wrong that most custom sections are not directly printable, though. Besides sourceMappingURL, do you know which ones will be printed by this code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree that most (aside from sourceMappingURL and asyncify) are not directly printable, but I don't see how it's worse than the current status quo. Basically, this PR at least gives a way to debug printable sections, and leaves the rest up to the implementer, like they were left before. I'm not going for the general implementation here, just fixing one thing at a time and trying to find a way to at least debug sections we already know how to print.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does seem reasonable to me. Even if it works for just one section, it's simple and helpful. We can always add custom section printing logics later.

One thing that does worry me though is what happens if the section is extremely large (but printable), like if it contains raw data that happens to be in the ascii range (like 10MB of a). Maybe there should be a limit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh yeah, I thought about that, but can't think of such an example section for now and it requires thinking about which limit would be reasonable :D If you have ideas (200 bytes? 1000? More?), I'm happy to add such limit. Or we can wait until someone runs into a real-world issue with this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, can wait for a concrete issue I guess...

This helps with debugging human-readable sections like sourceMappingURL.
@RReverser RReverser force-pushed the print-custom-sections branch from 04e2ca2 to 30cb271 Compare September 4, 2019 10:34
@kripken kripken merged commit 77fa398 into WebAssembly:master Sep 6, 2019
@RReverser RReverser deleted the print-custom-sections branch September 6, 2019 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants