-
Notifications
You must be signed in to change notification settings - Fork 758
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
Conversation
P.S. Waiting for CI as I'm not sure I updated all the necessary tests (had some spurious failures on Windows). |
67b5a8f
to
04e2ca2
Compare
Should be good to go now. |
Makes me wonder why custom sections aren't part of text format in the first place. Considering that there's |
@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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
04e2ca2
to
30cb271
Compare
This helps with debugging human-readable sections like sourceMappingURL.