This is an archive of the discontinued LLVM Phabricator instance.

[lld][WebAssembly] Add initial support for -Map/--print-map
ClosedPublic

Authored by sbc100 on Mar 31 2020, 5:41 PM.

Diff Detail

Event Timeline

sbc100 created this revision.Mar 31 2020, 5:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 5:41 PM
sbc100 updated this revision to Diff 254058.Mar 31 2020, 5:42 PM

clang format

sbc100 added a reviewer: ruiu.Mar 31 2020, 5:50 PM
sbc100 updated this revision to Diff 254062.Mar 31 2020, 5:51 PM

Fix test

sbc100 updated this revision to Diff 254063.Mar 31 2020, 5:53 PM

Remove comments

ruiu added inline comments.Mar 31 2020, 7:53 PM
lld/wasm/MapFile.cpp
128

Since wasm functions don't have addresses, you decided to print out "0" as an address, right? I wonder if we should something other than "0" (like "-") since "0" is still a valid address.

sbc100 updated this revision to Diff 254274.Apr 1 2020, 12:29 PM

Improve address and file offset information

ruiu added inline comments.Apr 12 2020, 8:19 PM
lld/test/wasm/map-file.s
28

Hmm, what is "Off"? It looks like it is monotonically increasing, but I'm not sure what these numbers are.

lld/wasm/MapFile.cpp
125–126

Since w is always 8, you can output a plain text instead of using right_justify.

sbc100 updated this revision to Diff 257486.Apr 14 2020, 1:51 PM
sbc100 marked 3 inline comments as done.

feedback

lld/test/wasm/map-file.s
28

These are the offset in the wasm module on disk.

lld/wasm/MapFile.cpp
125–126

Good point. Done.

ruiu added inline comments.Apr 14 2020, 9:08 PM
lld/test/wasm/map-file.s
28

Maybe a noob question, but is that significant? For ELF and COFF, we emit an offset within an output section for each input section, but we don't print out an offset within a file.

sbc100 marked an inline comment as done.Apr 15 2020, 9:26 AM
sbc100 added inline comments.
lld/test/wasm/map-file.s
28

It hard to say what the most useful information to present is. Do you have rational for why offset within an output section for each input section is useful? I happy to modify this PR to display that information instead. I imagine we could change this based on consumer feedback in the future too.

MaskRay added inline comments.
lld/test/wasm/map-file.s
4

--match-full-lines --strip-whitespace

--strip-whitespace alone still ignores leading or trailing spaces.

See test/ELF/map-file-64bit.s for an example

sbc100 updated this revision to Diff 257939.Apr 15 2020, 7:05 PM
sbc100 marked an inline comment as done.

feedback

sbc100 added inline comments.Apr 15 2020, 7:07 PM
lld/test/wasm/map-file.s
4

Sure thing.. I was copying lld/test/ELF/map-file.s which just uses -strict-whitespace.

At least it did until today when you changed it in cece7af58682a2122b108d7af270a31043ac1825 :)

Any reason you also changed all the comments from // to # in that change? I think we should try to be consistent across all the .s tests in lld so maybe we change all of them at once? (Assuming we prefer the hash mark).

Also, I looks like you didn't send it for review (or at least no here on phabicator)? Was that deliberate?

Sorry, I don't mean to sound picky.. happy for the feedback here.

MaskRay added inline comments.Apr 15 2020, 9:13 PM
lld/test/wasm/map-file.s
4

I think lld/test/ELF/map-file.s should have been fixed earlier before you copied it to wasm.

About // -> #, the description of that commit mentioned that since I had to change so many lines (adding leading spaces), just change the comments to use the more common way.

For other tests, it might just create churn if we do so. So I would not want to do that unless there is just motivation and I have to change a majority of lines (then optimizing for diff no longer counts)

Also, I looks like you didn't send it for review (or at least no here on phabicator)? Was that deliberate?

Sorry, I don't mean to sound picky.. happy for the feedback here.

Yes. It is NFC and likely community consensus plus I have made sufficient changes to lld/ELF. I probably would not want to do that if you did not copy it here.

sbc100 updated this revision to Diff 258369.Apr 17 2020, 10:47 AM
sbc100 marked an inline comment as done.

Use # over \\ is a comments in .s files

lld/test/wasm/map-file.s
28

Ping.

Would you prefer I output "offset with output section for each input section" ? Looking at test/ELF/map-file.s only see LMA and VMA (which I assume stands for virtual memory address?). I'm not sure what LMA has a logical correspondence in wasm. Is this what you call " offset with output section for each input section" ? For the test code LMA and VMA seems to be the same in all cases.

I'm happy to change this however you like but I also think its find to land as is and get feedback as to what information is the most useful to present.

sbc100 marked an inline comment as done.Apr 17 2020, 10:58 AM
sbc100 added inline comments.
lld/test/wasm/map-file.s
4

I think lld/test/ELF/map-file.s should have been fixed earlier before you copied it to wasm.

It looks like lld/test/ELF/map-file.s was only fixed 2 days ago. This PR has been open a lot longer than that. Of course it makes no difference at all, I'm just pointing this out for the record to explain why this change started life like this and that it was based on an existing test in the ELF linker.

MaskRay added inline comments.Apr 17 2020, 11:30 AM
lld/test/wasm/map-file.s
4

I might mistake your last comment as questioning my change... so I was defending myself. Sorry as after I reread the comments if seems that you did not mean for that...

28

Use CHECK-NEXT: if applicable.

sbc100 updated this revision to Diff 259729.Apr 23 2020, 3:12 PM
sbc100 marked an inline comment as done.

feedback

ping @ruiu , would you like to me to change the information that we print here?

MaskRay added inline comments.Apr 23 2020, 3:27 PM
lld/test/ELF/map-file.s
14 ↗(On Diff #259729)

I know some people prefer the double-dash options for FileCheck now. If you are going to add new lines or touching existing lines, you may use the double-dash form.

lld/wasm/MapFile.cpp
133

os.indent(8) << toString(chunk) << '\n'

lld/wasm/OutputSections.cpp
234 ↗(On Diff #259729)

This order shuffle is not needed.

lld/wasm/OutputSegment.h
41 ↗(On Diff #259729)

Add a comment what this means. It seems that outputSec is purely for debugging purposes (-M).

sbc100 updated this revision to Diff 259754.Apr 23 2020, 4:23 PM
sbc100 marked 4 inline comments as done.

feedback

lld/wasm/OutputSections.cpp
234 ↗(On Diff #259729)

This is to make it match the above two for loops where section->outputSec = this; is the first line.

lld/wasm/OutputSegment.h
41 ↗(On Diff #259729)

On further inspection I was able to revert this part.

sbc100 updated this revision to Diff 269982.Jun 10 2020, 3:31 PM
  • rebase

Can we pick this up again?

It looks like the only unresolved issues are the question of whether we should print section offsets or file offsets, and whether to put "-" or "0" for function addresses. I don't really have any opinion on the latter.
For the former question in general it's unfortunately slightly annoying, because for sections like data and debug info, section offsets are more relevant. But for code, VMs tend to print code addresses as file offsets.
But for the view mentioned above, (printing the offsets of each output section especially the official known sections such as CODE, DATA, etc where there can be only one), file offset probably makes the most sense to me.

MaskRay accepted this revision.Sep 11 2020, 7:00 PM
MaskRay added inline comments.
lld/wasm/MapFile.cpp
39
This revision is now accepted and ready to land.Sep 11 2020, 7:00 PM
sbc100 updated this revision to Diff 291423.Sep 12 2020, 4:11 PM

feedback

sbc100 added inline comments.Sep 12 2020, 4:12 PM
lld/wasm/MapFile.cpp
39

Interesting, thanks.

I guess I should update all the source files in lld/wasm to conform.

Sadly wasm:: is ambiguous so I had to use llvm::wasm::

This revision was landed with ongoing or failed builds.Sep 12 2020, 4:13 PM
This revision was automatically updated to reflect the committed changes.