[LTO] Fix assertion failed when flushing bitcode incrementally for LTO output.

[Enna1]

Summary

In https://reviews.llvm.org/D86905, we introduce an optimization, when lld emits LLVM bitcode,
we allow bitcode writer flush data to disk early when buffered data size is above some threshold.

But when --plugin-opt=emit-llvm and -o /dev/null are used,
lld will trigger assertion BytesRead >= 0 && static_cast<size_t>(BytesRead) == BytesFromDisk.
When we write output to /dev/null, BytesRead is zero, but at this program point BytesFromDisk is always non-zero.

This patch fix this assertion failed

[tejohnson]

Can you add a test?

[Enna1]

On default, command line option FlushThreshold is 512(M) defined in llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

static cl::opt<uint32_t> FlushThreshold(
    "bitcode-flush-threshold", cl::Hidden, cl::init(512),
    cl::desc("The threshold (unit M) for flushing LLVM bitcode."));

and AFAIK, there is no command line option for lld to modify FlushThreshold value.
So to reproduce this assertion failed in a tiny testcase, we need modify FlushThreshold to 0 manually in BitcodeWriter constructor:

BitcodeWriter::BitcodeWriter(SmallVectorImpl<char> &Buffer, raw_fd_stream *FS)
    : Buffer(Buffer), Stream(new BitstreamWriter(Buffer, FS, /*FlushThreshold*/0)) {
  writeBitcodeHeader(*Stream);
}

Then we can trigger this assertion failed:

$ clang -c -flto tu1.cpp -o tu1.o
$ clang -c -flto tu2.cpp -o tu2.o
$ clang -fuse-ld=lld -flto -Wl,--plugin-opt=emit-llvm tu1.o tu2.o -o /dev/null

The content of tu1.cpp and tu2.cpp:

// tu1.cpp
int unused(int a);
int probably_inlined(int a);
int main(int argc, const char *argv[]) {
  return probably_inlined(argc);
}

// tu2.cpp
int unused(int a) {
  return a + 1;
}
int probably_inlined(int a) {
  return a + 2;
}

[tejohnson]

In D112297#3080657, @Enna1 wrote:

On default, command line option FlushThreshold is 512(M) defined in llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

static cl::opt<uint32_t> FlushThreshold(
    "bitcode-flush-threshold", cl::Hidden, cl::init(512),
    cl::desc("The threshold (unit M) for flushing LLVM bitcode."));

and AFAIK, there is no command line option for lld to modify FlushThreshold value.

You can set internal an llvm option such as this from lld via -mllvm -bitcode-flush-threshold=0, or from the clang driver via -Wl,-mllvm,-bitcode-flush-threshold=0

[Enna1]

Add a test.

[MaskRay]

Before openLTOOutputFile is called, the output may be non-existent.
Then if (!sys::fs::is_regular_file(file)) will disable the D86905 memory optimization.

The D86905 memory optimization relies on read(2) reading back bytes in the file (for the BackpatchWord operation).
However, read(2) on /dev/null just returns 0 and breaks the D86905 assumption.

Now the optimization looks subtle to me. The special file opening operation probably does not belong to lld/ELF. All LTO users need to write some code like this.
So hope @stephan.yichao.zhao can move the subtle code to BitstreamWriter.h.

[stephan.yichao.zhao]

In D112297#3091836, @MaskRay wrote:

Before openLTOOutputFile is called, the output may be non-existent.
Then if (!sys::fs::is_regular_file(file)) will disable the D86905 memory optimization.

The code eventually uses this check to decide if a file is seekable.
The windows branch checks if a file is a regular file because of pipe. For non-linux, I guess the lseek(/dev/null, 0) does not return -1 for some reason.
Is it possible to put the check here?

[MaskRay]

Thanks for the pointer. Do you have suggestion on how/where to fix the issue leveraging SupportsSeeking?

[stephan.yichao.zhao]

In D112297#3092055, @MaskRay wrote:

Thanks for the pointer. Do you have suggestion on how/where to fix the issue leveraging SupportsSeeking?

The spec of lseek says

The behavior of lseek() on devices which are incapable of seeking is implementation-defined. The value of the file offset associated with such a device is undefined.

Is /dev/null a device-like file? If so, using lseek is not a stable approach.

Maybe using the following for both WIN32 and non-WIN32 is better

SupportsSeeking = !EC && Status.type() == sys::fs::file_type::regular_file;

[stephan.yichao.zhao]

To limit the restriction to only devices-like file, this says character and block types are devices, using LLVM enums, it could be like this around here:

sys::fs::file_status Status;
std::error_code EC = status(FD, Status);
if (EC ||
    Status.type() == sys::fs::file_type::block_file || 
    Status.type() == sys::fs::file_type::character_file) {
  SupportsSeeking = false;
} else {
  off_t loc = ::lseek(FD, 0, SEEK_CUR);
  SupportsSeeking = loc != (off_t)-1;
}

[MaskRay]

In D112297#3092088, @stephan.yichao.zhao wrote:

In D112297#3092055, @MaskRay wrote:

Thanks for the pointer. Do you have suggestion on how/where to fix the issue leveraging SupportsSeeking?

The spec of lseek says

The behavior of lseek() on devices which are incapable of seeking is implementation-defined. The value of the file offset associated with such a device is undefined.

Is /dev/null a device-like file? If so, using lseek is not a stable approach.

Maybe using the following for both WIN32 and non-WIN32 is better

SupportsSeeking = !EC && Status.type() == sys::fs::file_type::regular_file;

Agreed. Does this change address this patch's issue (ld.lld --plugin-opt=emit-llvm -mllvm -bitcode-flush-threshold=0 -o /dev/null %t.o)?

Can you post a patch? (Patching lld/ELF/LTO.cpp gives me a feeling working around a pitfall which might well hit others, so fixing the source would be nice.)

[stephan.yichao.zhao]

In D112297#3092429, @MaskRay wrote:

Agreed. Does this change address this patch's issue (ld.lld --plugin-opt=emit-llvm -mllvm -bitcode-flush-threshold=0 -o /dev/null %t.o)?

Can you post a patch? (Patching lld/ELF/LTO.cpp gives me a feeling working around a pitfall which might well hit others, so fixing the source would be nice.)

It needs some test to confirm. I probably do not have time to try this until this weekend.
@Enna1: If you have time, can you please verify if the suggested change works with your local test cases, and update this change if it works?

[Enna1]

In D112297#3092443, @stephan.yichao.zhao wrote:

In D112297#3092429, @MaskRay wrote:

Agreed. Does this change address this patch's issue (ld.lld --plugin-opt=emit-llvm -mllvm -bitcode-flush-threshold=0 -o /dev/null %t.o)?

Can you post a patch? (Patching lld/ELF/LTO.cpp gives me a feeling working around a pitfall which might well hit others, so fixing the source would be nice.)

It needs some test to confirm. I probably do not have time to try this until this weekend.
@Enna1: If you have time, can you please verify if the suggested change works with your local test cases, and update this change if it works?

Sure, I'll verify if the suggested change can fix the assertion failure with my local testcases.
If it works , I'll update this change.
Thanks @stephan.yichao.zhao @MaskRay

[Enna1]

Update patch as @stephan.yichao.zhao @MaskRay suggested.
For both WIN32 and non-WIN32, use the following code to determine SupportsSeeking

off_t loc = ::lseek(FD, 0, SEEK_CUR);
sys::fs::file_status Status;
std::error_code EC = status(FD, Status);
if (EC ||
    Status.type() == sys::fs::file_type::block_file || 
    Status.type() == sys::fs::file_type::character_file) {
  SupportsSeeking = false;
} else {
  SupportsSeeking = loc != (off_t)-1;
}

[Enna1]

The latest change fails two tests:

• raw_pwrite_ostreamTest.TestDevNull
• LTO/ARM/inline-asm.ll

It seems making raw_fd_ostream(/dev/null) do not support seeking breaks logic elsewhere ...

[stephan.yichao.zhao]

Thank you for testing this.

The test cases fail because the code seeks on /dev/null. Somehow /dev/null can be seeked to the current position, but not others, while BitcodeWriter fails because it seeks /dev/null to non-current position.

BitcodeWriter relies on raw_fd_stream. If the creation of raw_fd_stream returns an error, BitcodeWriter will gets only raw_fd_ostream. raw_fd_stream uses supportsSeek to check if returning an error.

raw_fd_stream was introduced for BitcodeWriter to support that optimization. So I am suggesting we put your original change here like this

sys::fs::file_status Status;
std::error_code EC = status(get_fd(), Status);
if (EC || Status.type() != sys::fs::file_type::regular_file)
    EC = std::make_error_code(std::errc::invalid_argument);

[Enna1]

Update patch.
Introduce an is_regular_file check in raw_fd_stream constructor, if not a regular file, return an error code. And BitcodeWriter will fallback use raw_fd_ostream.
This looks good under my tests.
Thanks @stephan.yichao.zhao

[stephan.yichao.zhao]

LGTM. Thank you! will leave this to @MaskRay for the final check.

[steven_wu]

llvm/lib/Support/raw_ostream.cpp

918 ↗

(On Diff #383221)

Would it be better to cache the result in the constructor for raw_fd_ostream? Adding another status call might not be optimal.

@dexonsmith Do you have any concerns over this?

[dexonsmith]

llvm/lib/Support/raw_ostream.cpp

918 ↗

(On Diff #383221)

Agreed that adding a stat is potentially expensive; is there already one in the other raw_fd_ostream constructor? Why not reuse it?

[Enna1]

Thanks for your review @steven_wu @dexonsmith

What about add a bool IsRegularFile field like bool SupportsSeeking in raw_fd_ostream, and change the code around here like this:

  // Get the starting position.
  off_t loc = ::lseek(FD, 0, SEEK_CUR);
  sys::fs::file_status Status;
  std::error_code EC = status(FD, Status);
  IsRegularFile = Status.type() == sys::fs::file_type::regular_file;
#ifdef _WIN32
  // MSVCRT's _lseek(SEEK_CUR) doesn't return -1 for pipes.
  SupportsSeeking = !EC && IsRegularFile;
#else
  SupportsSeeking = loc != (off_t)-1;
#endif

So we can directly use IsRegularFile in raw_fd_stream constructor.

[Enna1]

friendly ping :)

[MaskRay]

You can upload a new diff.

I support that SupportsSeeking = additionally checks Status.type() == sys::fs::file_type::regular_file, so that Windows/non-Windows code paths are unified.

[Enna1]

Update patch.
Add a bool IsRegularFile field like bool SupportsSeeking in raw_fd_ostream.
Cache the result Status.type() == sys::fs::file_type::regular_file in the constructor for raw_fd_ostream,
and replace checking SupportsSeeking with checking IsRegularFile in raw_fd_stream constructor, if not a regular file, return an error code.

We can not add an additional check Status.type() == sys::fs::file_type::regular_file for SupportsSeeking, this will fail another two tests, @stephan.yichao.zhao wrote an explanation for this.

In D112297#3112195, @MaskRay wrote:

You can upload a new diff.

I support that SupportsSeeking = additionally checks Status.type() == sys::fs::file_type::regular_file, so that Windows/non-Windows code paths are unified.

Here :)

In D112297#3093809, @stephan.yichao.zhao wrote:

Thank you for testing this.

The test cases fail because the code seeks on /dev/null. Somehow /dev/null can be seeked to the current position, but not others, while BitcodeWriter fails because it seeks /dev/null to non-current position.

BitcodeWriter relies on raw_fd_stream. If the creation of raw_fd_stream returns an error, BitcodeWriter will gets only raw_fd_ostream. raw_fd_stream uses supportsSeek to check if returning an error.

raw_fd_stream was introduced for BitcodeWriter to support that optimization. So I am suggesting we put your original change here like this

sys::fs::file_status Status;
std::error_code EC = status(get_fd(), Status);
if (EC || Status.type() != sys::fs::file_type::regular_file)
    EC = std::make_error_code(std::errc::invalid_argument);

[Enna1]

ping :)

[Enna1]

Kindly ping :) @steven_wu @dexonsmith @MaskRay

[MaskRay]

lld/test/ELF/lto/emit-llvm_assertion_failure.ll

8

ld.lld has very little to do with this LTO regression. It'd be good to use llvm-lto2 or llvm-lto to test this, not ld.lld

llvm/lib/Support/raw_ostream.cpp

651 ↗

(On Diff #385337)

Why not SupportsSeeking = !EC && IsRegularFile; for non-Windows as well?

[Enna1]

Update Patch.

Why not SupportsSeeking = !EC && IsRegularFile; for non-Windows as well?

Use SupportsSeeking = !EC && IsRegularFile; for both Windows and non-Windows.

ld.lld has very little to do with this LTO regression. It'd be good to use llvm-lto2 or llvm-lto to test this, not ld.lld

Cause https://reviews.llvm.org/D86905 the special file opening operation is only used in lld/ELF, so for now we can only test this bug in ld.lld.

[MaskRay]

lld/test/ELF/lto/emit-llvm_assertion_failture.ll

1 ↗

(On Diff #396741)

;;

1 ↗

(On Diff #396741)

Use - as filename separator

Suggest: move the RUN line and the comment into emit-llvm.ll. No need for a new test file.

15 ↗

(On Diff #396741)

delete trailing blank line

[Enna1]

Address review comments.

[MaskRay]

Thanks!

[Enna1]

Revert using SupportsSeeking = !EC && IsRegularFile; for both Windows and non-Windows, which makes raw_pwrite_ostreamTest.TestDevNull and LTO/ARM/inline-asm.ll failed.

lld/ELF/LTO.cpp

static std::unique_ptr<raw_fd_ostream> openFile(StringRef file) {
   return ret;
 }
 
 // The merged bitcode after LTO is large. Try opening a file stream that
 // supports reading, seeking and writing. Such a file allows BitcodeWriter to
 // flush buffered data to reduce memory consumption. If this fails, open a file
 // stream that supports only write.
 static std::unique_ptr<raw_fd_ostream> openLTOOutputFile(StringRef file) {
+  // Don't flush bitcode incrementally if the output is a special file.
+  // This handles things like '-o /dev/null'
+  if (!sys::fs::is_regular_file(file))
+    return openFile(file);
   std::error_code ec;
   std::unique_ptr<raw_fd_ostream> fs =
       std::make_unique<raw_fd_stream>(file, ec);
   if (!ec)
     return fs;
   return openFile(file);
 }
 

lld/test/ELF/lto/emit-llvm_assertion_failure.ll

+; Make sure that if --plugin-opt=emit-llvm is enabled and the output
+; is /dev/null, it should not cause a assertion failure in bitcode writer.
+
+; REQUIRES: x86
+
+; RUN: opt -module-hash -module-summary %s -o %t.o
+; RUN: ld.lld --plugin-opt=emit-llvm -mllvm -bitcode-flush-threshold=0 -o /dev/null %t.o
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @main() {
+  ret void
+}