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

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lib/MC/WasmObjectWriter.cpp:405
+  if (!Size)
+    return;
+
----------------
sbc100 wrote:
> efriedma wrote:
> > sbc100 wrote:
> > > sbc100 wrote:
> > > > efriedma wrote:
> > > > > It seems unsafe to assume all streams that don't support tell() are /dev/null.
> > > > 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().
> > > > 
> > > There is precedence at raw_ostream.h:352 for handling a zero result from tell() to mean /dev/null.
> > 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.
> 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?
> 
> 
Sure.


================
Comment at: lib/MC/WasmObjectWriter.cpp:408
+  Size -= Section.PayloadOffset;
+  if (Size && uint32_t(Size) != Size)
     report_fatal_error("section size does not fit in a uint32_t");
----------------
"Size &&" doesn't seem useful here?  I mean, if Size == 0, the overflow test will pass.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57479/new/

https://reviews.llvm.org/D57479