Page MenuHomePhabricator

[WebAssembly] Fix MCNullStreamer support
AbandonedPublic

Authored by eush on Nov 15 2018, 8:18 PM.

Details

Summary

This fixes crashes in AsmPrinter when using MCNullStreamer
(-filetype=null).

With MCNullStreamer, getTargetStreamer returns null, so this
patch adds some null checks to avoid a crash.

Diff Detail

Event Timeline

eush created this revision.Nov 15 2018, 8:18 PM
eush edited the summary of this revision. (Show Details)Nov 15 2018, 8:26 PM
sbc100 added inline comments.Nov 16 2018, 9:00 AM
test/CodeGen/WebAssembly/null-streamer.ll
11

Can you simplify this test, drop the trow and remove the exception arguments from llc?

eush added inline comments.Nov 16 2018, 2:16 PM
test/CodeGen/WebAssembly/null-streamer.ll
11

Can I wonder why? A simpler test would only exercise changes in EmitFunctionBodyStart and wouldn't need any changes in EmitEndOfAsmFile.

sbc100 accepted this revision.Nov 16 2018, 2:48 PM
sbc100 added inline comments.
test/CodeGen/WebAssembly/null-streamer.ll
11

Oh I see. I just found a it a little strange to have those options enabled, but I suppose it makes sense. Alternatively you could call alloca in order to trigger the use of __stack_pointer.

Also, if you want to be sure you excersise those parts maybe you would want to at a CHECK: for the .globaltype and/or .eventtype at the end?

This revision is now accepted and ready to land.Nov 16 2018, 2:48 PM
aheejin added a comment.EditedNov 16 2018, 3:18 PM

Thanks for doing this! By the way, every time we use getTargetStreamer(), are we required do this? I checked some other *AsmPrinter.cpp files for other targets and it seems they are not doing this, but they can run -filetype=null with no problem. I wonder what makes the difference?

test/CodeGen/WebAssembly/null-streamer.ll
11

This is nullstreamer, so I guess it wouldn't print anything so we can't check.

I opened D54660. I think this way we don't need to use if (WebAssemblyTargetStreamer *TS = getTargetStreamer()) ... every time we use getTargetStreamer(). What do you think?

eush added a comment.Nov 16 2018, 11:30 PM

I opened D54660. I think this way we don't need to use if (WebAssemblyTargetStreamer *TS = getTargetStreamer()) ... every time we use getTargetStreamer(). What do you think?

I think that's great! I didn't know about RegisterNullTargetStreamer - it looks to me like the correct way to support MCNullStreamer.

test/CodeGen/WebAssembly/null-streamer.ll
11

This is nullstreamer, so I guess it wouldn't print anything so we can't check.

That is unless we add a second RUN line with a default streamer, just for the checks.

aheejin added inline comments.Nov 17 2018, 2:22 PM
test/CodeGen/WebAssembly/null-streamer.ll
11

This might be a better idea, having a second RUN line. I'll fix that in D54660,

eush abandoned this revision.Nov 17 2018, 9:09 PM

Abandoned in favor of D54660.