From 93c9a3bd387f561dd07e4811602b0755276444ee Mon Sep 17 00:00:00 2001 From: Jeremy Faller Date: Fri, 17 Apr 2020 16:07:45 -0400 Subject: [PATCH] [dev.link] cmd/link: remove buffered file I/O from OutBuf Recreation of CL 228317. The problem with that original CL was a late requested change, reordering reloc and asmb, resulting in symbols having stale pointers to their data. I've fixed this by preallocating the heap variable in OutBuf for platforms w/o mmap. Change-Id: Icdb392ac2c8d6518830f4c84cf422e78b8ab68c7 Reviewed-on: https://go-review.googlesource.com/c/go/+/228782 Run-TryBot: Jeremy Faller TryBot-Result: Gobot Gobot Reviewed-by: Than McIntosh --- src/cmd/link/internal/amd64/asm.go | 5 - src/cmd/link/internal/arm/asm.go | 4 - src/cmd/link/internal/arm64/asm.go | 4 - src/cmd/link/internal/ld/data.go | 8 +- src/cmd/link/internal/ld/main.go | 14 +-- src/cmd/link/internal/ld/outbuf.go | 110 ++++++---------------- src/cmd/link/internal/ld/outbuf_nommap.go | 13 +-- src/cmd/link/internal/ld/outbuf_test.go | 1 + src/cmd/link/internal/ld/xcoff.go | 3 - src/cmd/link/internal/mips/asm.go | 2 - src/cmd/link/internal/mips64/asm.go | 4 - src/cmd/link/internal/ppc64/asm.go | 5 - src/cmd/link/internal/riscv64/asm.go | 2 - src/cmd/link/internal/s390x/asm.go | 2 - src/cmd/link/internal/wasm/asm.go | 2 - src/cmd/link/internal/x86/asm.go | 5 - 16 files changed, 47 insertions(+), 137 deletions(-) diff --git a/src/cmd/link/internal/amd64/asm.go b/src/cmd/link/internal/amd64/asm.go index 5c4ffe19c25..d26a9a234cc 100644 --- a/src/cmd/link/internal/amd64/asm.go +++ b/src/cmd/link/internal/amd64/asm.go @@ -756,7 +756,6 @@ func asmb2(ctxt *ld.Link) { if ctxt.IsELF { ctxt.Out.SeekSet(symo) ld.Asmelfsym(ctxt) - ctxt.Out.Flush() ctxt.Out.Write(ld.Elfstrdat) if ctxt.LinkMode == ld.LinkExternal { @@ -766,13 +765,11 @@ func asmb2(ctxt *ld.Link) { case objabi.Hplan9: ld.Asmplan9sym(ctxt) - ctxt.Out.Flush() sym := ctxt.Syms.Lookup("pclntab", 0) if sym != nil { ld.Lcsize = int32(len(sym.P)) ctxt.Out.Write(sym.P) - ctxt.Out.Flush() } case objabi.Hwindows: @@ -817,8 +814,6 @@ func asmb2(ctxt *ld.Link) { case objabi.Hwindows: ld.Asmbpe(ctxt) } - - ctxt.Out.Flush() } func tlsIEtoLE(s *sym.Symbol, off, size int) { diff --git a/src/cmd/link/internal/arm/asm.go b/src/cmd/link/internal/arm/asm.go index f3d1262879a..e9eea5ce2c9 100644 --- a/src/cmd/link/internal/arm/asm.go +++ b/src/cmd/link/internal/arm/asm.go @@ -816,7 +816,6 @@ func asmb2(ctxt *ld.Link) { default: if ctxt.IsELF { ld.Asmelfsym(ctxt) - ctxt.Out.Flush() ctxt.Out.Write(ld.Elfstrdat) if ctxt.LinkMode == ld.LinkExternal { @@ -826,13 +825,11 @@ func asmb2(ctxt *ld.Link) { case objabi.Hplan9: ld.Asmplan9sym(ctxt) - ctxt.Out.Flush() sym := ctxt.Syms.Lookup("pclntab", 0) if sym != nil { ld.Lcsize = int32(len(sym.P)) ctxt.Out.Write(sym.P) - ctxt.Out.Flush() } case objabi.Hwindows: @@ -863,7 +860,6 @@ func asmb2(ctxt *ld.Link) { ld.Asmbpe(ctxt) } - ctxt.Out.Flush() if *ld.FlagC { fmt.Printf("textsize=%d\n", ld.Segtext.Filelen) fmt.Printf("datsize=%d\n", ld.Segdata.Filelen) diff --git a/src/cmd/link/internal/arm64/asm.go b/src/cmd/link/internal/arm64/asm.go index 053b8d119d9..46bda74c4c5 100644 --- a/src/cmd/link/internal/arm64/asm.go +++ b/src/cmd/link/internal/arm64/asm.go @@ -866,7 +866,6 @@ func asmb2(ctxt *ld.Link) { default: if ctxt.IsELF { ld.Asmelfsym(ctxt) - ctxt.Out.Flush() ctxt.Out.Write(ld.Elfstrdat) if ctxt.LinkMode == ld.LinkExternal { @@ -876,13 +875,11 @@ func asmb2(ctxt *ld.Link) { case objabi.Hplan9: ld.Asmplan9sym(ctxt) - ctxt.Out.Flush() sym := ctxt.Syms.Lookup("pclntab", 0) if sym != nil { ld.Lcsize = int32(len(sym.P)) ctxt.Out.Write(sym.P) - ctxt.Out.Flush() } case objabi.Hdarwin: @@ -915,7 +912,6 @@ func asmb2(ctxt *ld.Link) { ld.Asmbmacho(ctxt) } - ctxt.Out.Flush() if *ld.FlagC { fmt.Printf("textsize=%d\n", ld.Segtext.Filelen) fmt.Printf("datsize=%d\n", ld.Segdata.Filelen) diff --git a/src/cmd/link/internal/ld/data.go b/src/cmd/link/internal/ld/data.go index 3979880cf4d..36708ee5d1e 100644 --- a/src/cmd/link/internal/ld/data.go +++ b/src/cmd/link/internal/ld/data.go @@ -32,7 +32,6 @@ package ld import ( - "bufio" "bytes" "cmd/internal/gcprog" "cmd/internal/objabi" @@ -958,11 +957,10 @@ func Datblk(ctxt *Link, out *OutBuf, addr, size int64) { // Used only on Wasm for now. func DatblkBytes(ctxt *Link, addr int64, size int64) []byte { - buf := bytes.NewBuffer(make([]byte, 0, size)) - out := &OutBuf{w: bufio.NewWriter(buf)} + buf := make([]byte, size) + out := &OutBuf{heap: buf} writeDatblkToOutBuf(ctxt, out, addr, size) - out.Flush() - return buf.Bytes() + return buf } func writeDatblkToOutBuf(ctxt *Link, out *OutBuf, addr int64, size int64) { diff --git a/src/cmd/link/internal/ld/main.go b/src/cmd/link/internal/ld/main.go index dd089e6efa5..4735b91b352 100644 --- a/src/cmd/link/internal/ld/main.go +++ b/src/cmd/link/internal/ld/main.go @@ -319,14 +319,12 @@ func Main(arch *sys.Arch, theArch Arch) { // for which we have computed the size and offset, in a // mmap'd region. The second part writes more content, for // which we don't know the size. - var outputMmapped bool if ctxt.Arch.Family != sys.Wasm { // Don't mmap if we're building for Wasm. Wasm file // layout is very different so filesize is meaningless. - err := ctxt.Out.Mmap(filesize) - outputMmapped = err == nil - } - if outputMmapped { + if err := ctxt.Out.Mmap(filesize); err != nil { + panic(err) + } // Asmb will redirect symbols to the output file mmap, and relocations // will be applied directly there. bench.Start("Asmb") @@ -344,10 +342,8 @@ func Main(arch *sys.Arch, theArch Arch) { bench.Start("Asmb2") thearch.Asmb2(ctxt) - if outputMmapped { - bench.Start("Munmap") - ctxt.Out.Munmap() - } + bench.Start("Munmap") + ctxt.Out.Close() // Close handles Munmapping if necessary. bench.Start("undef") ctxt.undef() diff --git a/src/cmd/link/internal/ld/outbuf.go b/src/cmd/link/internal/ld/outbuf.go index c36fc74a445..f043168f1a2 100644 --- a/src/cmd/link/internal/ld/outbuf.go +++ b/src/cmd/link/internal/ld/outbuf.go @@ -5,7 +5,6 @@ package ld import ( - "bufio" "cmd/internal/sys" "cmd/link/internal/sym" "encoding/binary" @@ -61,7 +60,6 @@ type OutBuf struct { buf []byte // backing store of mmap'd output file heap []byte // backing store for non-mmapped data - w *bufio.Writer name string f *os.File encbuf [8]byte // temp buffer used by WriteN methods @@ -78,7 +76,6 @@ func (out *OutBuf) Open(name string) error { } out.off = 0 out.name = name - out.w = bufio.NewWriter(f) out.f = f return nil } @@ -92,13 +89,11 @@ func NewOutBuf(arch *sys.Arch) *OutBuf { var viewError = errors.New("output not mmapped") func (out *OutBuf) View(start uint64) (*OutBuf, error) { - if !out.isMmapped() { - return nil, viewError - } return &OutBuf{ arch: out.arch, name: out.name, buf: out.buf, + heap: out.heap, off: int64(start), isView: true, }, nil @@ -110,10 +105,17 @@ func (out *OutBuf) Close() error { if out.isView { return viewCloseError } - out.Flush() + if out.isMmapped() { + return out.Munmap() + } if out.f == nil { return nil } + if len(out.heap) != 0 { + if _, err := out.f.Write(out.heap); err != nil { + return err + } + } if err := out.f.Close(); err != nil { return err } @@ -151,42 +153,28 @@ func (out *OutBuf) Munmap() error { // writing. When the mmapped section is full, we switch over the heap memory // for writing. func (out *OutBuf) writeLoc(lenToWrite int64) (int64, []byte) { - if !out.isMmapped() { - panic("shouldn't happen") - } - // See if we have enough space in the mmaped area. bufLen := int64(len(out.buf)) if out.off+lenToWrite <= bufLen { return out.off, out.buf } - // The heap variables aren't protected by a mutex. For now, just bomb if you - // try to use OutBuf in parallel. (Note this probably could be fixed.) - if out.isView { - panic("cannot write to heap in parallel") - } - // Not enough space in the mmaped area, write to heap area instead. heapPos := out.off - bufLen heapLen := int64(len(out.heap)) lenNeeded := heapPos + lenToWrite if lenNeeded > heapLen { // do we need to grow the heap storage? + // The heap variables aren't protected by a mutex. For now, just bomb if you + // try to use OutBuf in parallel. (Note this probably could be fixed.) + if out.isView { + panic("cannot write to heap in parallel") + } out.heap = append(out.heap, make([]byte, lenNeeded-heapLen)...) } return heapPos, out.heap } func (out *OutBuf) SeekSet(p int64) { - if p == out.off { - return - } - if !out.isMmapped() { - out.Flush() - if _, err := out.f.Seek(p, 0); err != nil { - Exitf("seeking to %d in %s: %v", p, out.name, err) - } - } out.off = p } @@ -195,33 +183,18 @@ func (out *OutBuf) Offset() int64 { } // Write writes the contents of v to the buffer. -// -// As Write is backed by a bufio.Writer, callers do not have -// to explicitly handle the returned error as long as Flush is -// eventually called. func (out *OutBuf) Write(v []byte) (int, error) { - if out.isMmapped() { - n := len(v) - pos, buf := out.writeLoc(int64(n)) - copy(buf[pos:], v) - out.off += int64(n) - return n, nil - } - n, err := out.w.Write(v) + n := len(v) + pos, buf := out.writeLoc(int64(n)) + copy(buf[pos:], v) out.off += int64(n) - return n, err + return n, nil } func (out *OutBuf) Write8(v uint8) { - if out.isMmapped() { - pos, buf := out.writeLoc(1) - buf[pos] = v - out.off++ - return - } - if err := out.w.WriteByte(v); err == nil { - out.off++ - } + pos, buf := out.writeLoc(1) + buf[pos] = v + out.off++ } // WriteByte is an alias for Write8 to fulfill the io.ByteWriter interface. @@ -256,16 +229,11 @@ func (out *OutBuf) Write64b(v uint64) { } func (out *OutBuf) WriteString(s string) { - if out.isMmapped() { - pos, buf := out.writeLoc(int64(len(s))) - n := copy(buf[pos:], s) - if n != len(s) { - log.Fatalf("WriteString truncated. buffer size: %d, offset: %d, len(s)=%d", len(out.buf), out.off, len(s)) - } - out.off += int64(n) - return + pos, buf := out.writeLoc(int64(len(s))) + n := copy(buf[pos:], s) + if n != len(s) { + log.Fatalf("WriteString truncated. buffer size: %d, offset: %d, len(s)=%d", len(out.buf), out.off, len(s)) } - n, _ := out.w.WriteString(s) out.off += int64(n) } @@ -297,26 +265,10 @@ func (out *OutBuf) WriteStringPad(s string, n int, pad []byte) { // edit to the symbol content. // If the output file is not Mmap'd, just writes the content. func (out *OutBuf) WriteSym(s *sym.Symbol) { - // NB: We inline the Write call for speediness. - if out.isMmapped() { - n := int64(len(s.P)) - pos, buf := out.writeLoc(n) - copy(buf[pos:], s.P) - out.off += n - s.P = buf[pos : pos+n] - s.Attr.Set(sym.AttrReadOnly, false) - } else { - n, _ := out.w.Write(s.P) - out.off += int64(n) - } -} - -func (out *OutBuf) Flush() { - var err error - if out.w != nil { - err = out.w.Flush() - } - if err != nil { - Exitf("flushing %s: %v", out.name, err) - } + n := int64(len(s.P)) + pos, buf := out.writeLoc(n) + copy(buf[pos:], s.P) + out.off += n + s.P = buf[pos : pos+n] + s.Attr.Set(sym.AttrReadOnly, false) } diff --git a/src/cmd/link/internal/ld/outbuf_nommap.go b/src/cmd/link/internal/ld/outbuf_nommap.go index 0b0ed912802..472fca22d74 100644 --- a/src/cmd/link/internal/ld/outbuf_nommap.go +++ b/src/cmd/link/internal/ld/outbuf_nommap.go @@ -6,10 +6,11 @@ package ld -import "errors" +func (out *OutBuf) Mmap(filesize uint64) error { + // We need space to put all the symbols before we apply relocations. + out.heap = make([]byte, filesize) + return nil +} -var errNotSupported = errors.New("mmap not supported") - -func (out *OutBuf) Mmap(filesize uint64) error { return errNotSupported } -func (out *OutBuf) munmap() { panic("unreachable") } -func (out *OutBuf) Msync() error { panic("unreachable") } +func (out *OutBuf) munmap() { panic("unreachable") } +func (out *OutBuf) Msync() error { panic("unreachable") } diff --git a/src/cmd/link/internal/ld/outbuf_test.go b/src/cmd/link/internal/ld/outbuf_test.go index aae206f511a..58f9b10cfa4 100644 --- a/src/cmd/link/internal/ld/outbuf_test.go +++ b/src/cmd/link/internal/ld/outbuf_test.go @@ -50,6 +50,7 @@ func TestWriteLoc(t *testing.T) { {100, 100, 0, 100, 100, 0, true}, {10, 10, 0, 100, 100, 0, true}, {10, 20, 10, 100, 110, 10, true}, + {0, 0, 0, 100, 100, 0, true}, } for i, test := range tests { diff --git a/src/cmd/link/internal/ld/xcoff.go b/src/cmd/link/internal/ld/xcoff.go index 4ff123e8cd6..9fe3669eee2 100644 --- a/src/cmd/link/internal/ld/xcoff.go +++ b/src/cmd/link/internal/ld/xcoff.go @@ -1389,7 +1389,6 @@ func (f *xcoffFile) writeLdrScn(ctxt *Link, globalOff uint64) { } f.loaderSize = off + uint64(stlen) - ctxt.Out.Flush() /* again for printing */ if !*flagA { @@ -1559,8 +1558,6 @@ func Asmbxcoff(ctxt *Link, fileoff int64) { // write string table xfile.stringTable.write(ctxt.Out) - ctxt.Out.Flush() - // write headers xcoffwrite(ctxt) } diff --git a/src/cmd/link/internal/mips/asm.go b/src/cmd/link/internal/mips/asm.go index b50112ad750..21a57ccbb0d 100644 --- a/src/cmd/link/internal/mips/asm.go +++ b/src/cmd/link/internal/mips/asm.go @@ -204,7 +204,6 @@ func asmb2(ctxt *ld.Link) { ctxt.Out.SeekSet(int64(symo)) ld.Asmelfsym(ctxt) - ctxt.Out.Flush() ctxt.Out.Write(ld.Elfstrdat) if ctxt.LinkMode == ld.LinkExternal { @@ -220,7 +219,6 @@ func asmb2(ctxt *ld.Link) { ld.Asmbelf(ctxt, int64(symo)) } - ctxt.Out.Flush() if *ld.FlagC { fmt.Printf("textsize=%d\n", ld.Segtext.Filelen) fmt.Printf("datsize=%d\n", ld.Segdata.Filelen) diff --git a/src/cmd/link/internal/mips64/asm.go b/src/cmd/link/internal/mips64/asm.go index 1b2914eea39..0a2a3c11f3e 100644 --- a/src/cmd/link/internal/mips64/asm.go +++ b/src/cmd/link/internal/mips64/asm.go @@ -223,7 +223,6 @@ func asmb2(ctxt *ld.Link) { default: if ctxt.IsELF { ld.Asmelfsym(ctxt) - ctxt.Out.Flush() ctxt.Out.Write(ld.Elfstrdat) if ctxt.LinkMode == ld.LinkExternal { @@ -233,13 +232,11 @@ func asmb2(ctxt *ld.Link) { case objabi.Hplan9: ld.Asmplan9sym(ctxt) - ctxt.Out.Flush() sym := ctxt.Syms.Lookup("pclntab", 0) if sym != nil { ld.Lcsize = int32(len(sym.P)) ctxt.Out.Write(sym.P) - ctxt.Out.Flush() } } } @@ -268,7 +265,6 @@ func asmb2(ctxt *ld.Link) { ld.Asmbelf(ctxt, int64(symo)) } - ctxt.Out.Flush() if *ld.FlagC { fmt.Printf("textsize=%d\n", ld.Segtext.Filelen) fmt.Printf("datsize=%d\n", ld.Segdata.Filelen) diff --git a/src/cmd/link/internal/ppc64/asm.go b/src/cmd/link/internal/ppc64/asm.go index d86738538d9..0e3a6914322 100644 --- a/src/cmd/link/internal/ppc64/asm.go +++ b/src/cmd/link/internal/ppc64/asm.go @@ -1130,7 +1130,6 @@ func asmb2(ctxt *ld.Link) { default: if ctxt.IsELF { ld.Asmelfsym(ctxt) - ctxt.Out.Flush() ctxt.Out.Write(ld.Elfstrdat) if ctxt.LinkMode == ld.LinkExternal { @@ -1140,18 +1139,15 @@ func asmb2(ctxt *ld.Link) { case objabi.Hplan9: ld.Asmplan9sym(ctxt) - ctxt.Out.Flush() sym := ctxt.Syms.Lookup("pclntab", 0) if sym != nil { ld.Lcsize = int32(len(sym.P)) ctxt.Out.Write(sym.P) - ctxt.Out.Flush() } case objabi.Haix: // symtab must be added once sections have been created in ld.Asmbxcoff - ctxt.Out.Flush() } } @@ -1180,7 +1176,6 @@ func asmb2(ctxt *ld.Link) { ld.Asmbxcoff(ctxt, int64(fileoff)) } - ctxt.Out.Flush() if *ld.FlagC { fmt.Printf("textsize=%d\n", ld.Segtext.Filelen) fmt.Printf("datsize=%d\n", ld.Segdata.Filelen) diff --git a/src/cmd/link/internal/riscv64/asm.go b/src/cmd/link/internal/riscv64/asm.go index db3b602b848..51cc5980c89 100644 --- a/src/cmd/link/internal/riscv64/asm.go +++ b/src/cmd/link/internal/riscv64/asm.go @@ -142,7 +142,6 @@ func asmb2(ctxt *ld.Link) { ctxt.Out.SeekSet(int64(symo)) ld.Asmelfsym(ctxt) - ctxt.Out.Flush() ctxt.Out.Write(ld.Elfstrdat) if ctxt.LinkMode == ld.LinkExternal { @@ -157,7 +156,6 @@ func asmb2(ctxt *ld.Link) { default: ld.Errorf(nil, "unsupported operating system") } - ctxt.Out.Flush() if *ld.FlagC { fmt.Printf("textsize=%d\n", ld.Segtext.Filelen) diff --git a/src/cmd/link/internal/s390x/asm.go b/src/cmd/link/internal/s390x/asm.go index 5d55a190726..9b6be28421f 100644 --- a/src/cmd/link/internal/s390x/asm.go +++ b/src/cmd/link/internal/s390x/asm.go @@ -515,7 +515,6 @@ func asmb2(ctxt *ld.Link) { ctxt.Out.SeekSet(int64(symo)) ld.Asmelfsym(ctxt) - ctxt.Out.Flush() ctxt.Out.Write(ld.Elfstrdat) if ctxt.LinkMode == ld.LinkExternal { @@ -531,7 +530,6 @@ func asmb2(ctxt *ld.Link) { ld.Asmbelf(ctxt, int64(symo)) } - ctxt.Out.Flush() if *ld.FlagC { fmt.Printf("textsize=%d\n", ld.Segtext.Filelen) fmt.Printf("datsize=%d\n", ld.Segdata.Filelen) diff --git a/src/cmd/link/internal/wasm/asm.go b/src/cmd/link/internal/wasm/asm.go index 550ed5bc3c4..7f8742d0082 100644 --- a/src/cmd/link/internal/wasm/asm.go +++ b/src/cmd/link/internal/wasm/asm.go @@ -187,8 +187,6 @@ func asmb2(ctxt *ld.Link) { if !*ld.FlagS { writeNameSec(ctxt, len(hostImports), fns) } - - ctxt.Out.Flush() } func lookupType(sig *wasmFuncType, types *[]*wasmFuncType) uint32 { diff --git a/src/cmd/link/internal/x86/asm.go b/src/cmd/link/internal/x86/asm.go index cbc79c987b9..7dcdda0fa80 100644 --- a/src/cmd/link/internal/x86/asm.go +++ b/src/cmd/link/internal/x86/asm.go @@ -646,7 +646,6 @@ func asmb2(ctxt *ld.Link) { default: if ctxt.IsELF { ld.Asmelfsym(ctxt) - ctxt.Out.Flush() ctxt.Out.Write(ld.Elfstrdat) if ctxt.LinkMode == ld.LinkExternal { @@ -656,13 +655,11 @@ func asmb2(ctxt *ld.Link) { case objabi.Hplan9: ld.Asmplan9sym(ctxt) - ctxt.Out.Flush() sym := ctxt.Syms.Lookup("pclntab", 0) if sym != nil { ld.Lcsize = int32(len(sym.P)) ctxt.Out.Write(sym.P) - ctxt.Out.Flush() } case objabi.Hwindows: @@ -702,6 +699,4 @@ func asmb2(ctxt *ld.Link) { case objabi.Hwindows: ld.Asmbpe(ctxt) } - - ctxt.Out.Flush() }