This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] MC: Fix for outputing wasm object to /dev/null
ClosedPublic

Authored by sbc100 on Jan 30 2019, 3:10 PM.

Diff Detail

Event Timeline

sbc100 created this revision.Jan 30 2019, 3:10 PM
efriedma added inline comments.
lib/MC/WasmObjectWriter.cpp
405

It seems unsafe to assume all streams that don't support tell() are /dev/null.

sbc100 marked an inline comment as done.Jan 30 2019, 4:03 PM
sbc100 added inline comments.
lib/MC/WasmObjectWriter.cpp
405

This code relies on OS.tell() working... if OS.tell() doesn't work should we abort instead? Then we would break the /dev/null. Are there other raw_pwrite_stream's that don't support OS.tell()? It seems like the ability to pwrite would normally come with the ability to tell().

sbc100 marked an inline comment as done.Jan 30 2019, 4:06 PM
sbc100 added inline comments.
lib/MC/WasmObjectWriter.cpp
405

There is precedence at raw_ostream.h:352 for handling a zero result from tell() to mean /dev/null.

efriedma added inline comments.Jan 30 2019, 5:27 PM
lib/MC/WasmObjectWriter.cpp
405

Can we handle this in some more consistent way?

I guess it's an issue with the way we detect whether a device supports seeking. For /dev/null and friends, lstat() with any offset technically succeeds, but it doesn't actually seek anywhere: the resulting offset is 0. We should probably fix our detection code to recognize that and set supportsSeeking() to false, instead of scattering weird special cases across LLVM. Maybe just reuse the Windows codepath in raw_fd_ostream::raw_fd_ostream that checks the fd refers to a file.

I guess that means we would end up buffering the entire file in some cases, due to supportsSeeking() checks; not sure how important that is.

If we do decide to go with this patch, you probably don't want to change the type of the "Size" variable; you're breaking the overflow check.

sbc100 updated this revision to Diff 184581.Jan 31 2019, 12:49 PM
  • Fix type of Size
sbc100 marked an inline comment as done.Jan 31 2019, 12:57 PM
sbc100 added inline comments.
lib/MC/WasmObjectWriter.cpp
405

I guess technically it doesn't support seeking so we should probably report supportsSeeking() correctly. However, I'd prefer to land this change than then follow up if thats OK?

efriedma accepted this revision.Jan 31 2019, 1:45 PM

LGTM

lib/MC/WasmObjectWriter.cpp
405

Sure.

408

"Size &&" doesn't seem useful here? I mean, if Size == 0, the overflow test will pass.

This revision is now accepted and ready to land.Jan 31 2019, 1:45 PM
This revision was automatically updated to reflect the committed changes.