This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][NFC] replace class Buffer/MemBuffer/FileBuffer with streams.
ClosedPublic

Authored by avl on Nov 8 2020, 2:58 AM.

Details

Summary

During D88827 it was requested to remove the local implementation
of Memory/File Buffers:

// TODO: refactor the buffer classes in LLVM to enable us to use them here
// directly.

This patch uses raw_ostream instead of Buffers. Generally, using streams
could allow us to reduce memory usages. No need to load all data into the
memory - the data could be streamed through a smaller buffer.
Thus, this patch uses raw_ostream as an interface for output data:

Error executeObjcopyOnBinary(CopyConfig &Config,
                             object::Binary &In,
                             raw_ostream &Out);

Note 1. This patch does not change the implementation of Writers
so that data would be directly stored into raw_ostream.
This is assumed to be done later.

Note 2. It would be better if Writers would be implemented in a such way
that data could be streamed without seeking/updating. If that would be
inconvenient then raw_ostream could be replaced with raw_pwrite_stream
to have a possibility to seek back and update file headers.
This is assumed to be done later if necessary.

Note 3. Current FileOutputBuffer allows using a memory-mapped file.
The raw_fd_ostream (which could be used if data should be stored in the file)
does not allow us to use a memory-mapped file. Memory map functionality
could be implemented for new kind of stream raw_mmap_stream.
This is assumed to be done later if necessary.

Diff Detail

Event Timeline

avl created this revision.Nov 8 2020, 2:58 AM
Herald added a project: Restricted Project. · View Herald Transcript
avl requested review of this revision.Nov 8 2020, 2:58 AM
avl edited the summary of this revision. (Show Details)Nov 8 2020, 3:00 AM
avl edited the summary of this revision. (Show Details)

I've skimmed this briefly, and it looks generally okay, but I've got too much else going on to give this a proper review. I'd prefer it if somebody else could take a look.

llvm/tools/llvm-objcopy/Util.h
9 ↗(On Diff #303705)

Here and below.

23–26 ↗(On Diff #303705)

No need for else after return.

43–44 ↗(On Diff #303705)

You don't need this.

grimar added a comment.EditedNov 16 2020, 3:53 AM

This also looks generally OK to me (after fixing style issues mentioned by James). Few more minor nits from me inline.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
138

I'd not use auto here. You can use an actual type instead.

142

The same, probably.

174

Do you need this helper? I think generally the direction is to get rid of error codes.
I am not sure if this method is usefull? You can just craft a better error message using createStringError in place.

215

Perhaps, link->rename?

avl added inline comments.Nov 16 2020, 4:11 AM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
138

these findBuildID, makeStringError, linkToBuildIdDir functions are not written from the scratch.
They were moved without modifications from ELF/ELFObjcopy.cpp.
I would prefer to not change them, since it would be separate refactoring not related to the current one.

Would it be OK, to make those changes as a separate patch?

grimar added inline comments.Nov 16 2020, 4:14 AM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
138

I see. Sorry, I haven't realized it. Sure, then lets keeps them as is for now.

avl updated this revision to Diff 305541.Nov 16 2020, 9:31 AM

addressed comments.

avl added a comment.Nov 23 2020, 1:37 AM

ping.

update for Note 3. It was considered to be not useful to implement reserve() method as fixed-size memory-mapped file(D91693). Instead it was suggested to implement resizable raw_mmap_ostream.
Thus, if we need to have memory mapped file for objcopy then we have this alternative - implement resizable raw_mmap_ostream.

In D91028#2410699, @avl wrote:

ping.

update for Note 3. It was considered to be not useful to implement reserve() method as fixed-size memory-mapped file(D91693). Instead it was suggested to implement resizable raw_mmap_ostream.
Thus, if we need to have memory mapped file for objcopy then we have this alternative - implement resizable raw_mmap_ostream.

FYI, I'm holding off reviewing this patch further (and I'd recommend others do the same personally) until we've sorted out D91693, as I think that will continue to have an impact on this patch.

@rupprecht @alexshap @grimar @dblaikie

Colleagues, so what is your opinion on this refactoring? Is it OK to use streams as output data for llvm-objcopy?

Hopefully some other folks with more investment in dsymutil chime in here, but at least:

This patch uses raw_ostream instead of Buffers. Generally, using streams could allow us to reduce memory usages. No need to load all data into the memory - the data could be streamed through a smaller buffer.

Aren't many MemoryBuffers backed by memory mapped files, which aren't necessarily causing real memory usage? Could that approach be used here?

That said, pretty sure LLVM's integrated assembler uses a pwrite stream to write out its object files, so if we can/if this patch does align with that method of output that seems OK.

I'm confused by the tenses in some of the notes in the patch description:

Note 2. It would be better if Writers would be implemented in a such way that data could be streamed without seeking/updating. If that would be inconvenient then raw_ostream could be replaced with raw_pwrite_stream to have a possibility to seek back and update file headers. This is assumed to be done later if necessary.

So this patch as proposed is not using a pwrite stream, it seems? "if Writers could be implemented in such a way that data could be streamed without seeking/updating" - how does that work today, without the patch, and how does the patch address that need to seek? I don't believe it's possible to write an ELF file without either knowing at least the sizes of all the sections up-front (you might be able to know these sizes without actually buffering the entire contents of the file) or seeking. Because the header needs to either contain an offset to the section table stored at the end of the file, where it knows the offsets/lengths of all the sections, or you write the section table at the start (so the header knows the offset up-front) and the table then needs to know the offsets of all the sections up-front.

avl added a comment.Dec 18 2020, 6:08 AM

Aren't many MemoryBuffers backed by memory mapped files, which aren't necessarily causing real memory usage? Could that approach be used here?

That is right. Many MemoryBuffers are backed by memory-mapped files, which aren't necessarily causing real memory usage.
But if we could not use a memory-mapped file then we do not have another way than loading the entire output file into the memory.
Using streams allows us to have another way than loading the entire file into the memory(for the cases when we could not use memory-mapped files).
f.e. there already exist the following code in the codebase:

Expected<std::unique_ptr<MemoryBuffer>>
object::writeUniversalBinaryToBuffer(ArrayRef<Slice> Slices) {
  SmallVector<char, 0> Buffer;
  raw_svector_ostream Out(Buffer);

  if (Error E = writeUniversalBinaryToStream(Slices, Out))  <<< entire output file is loaded into the memory
    return std::move(E);
    
  return std::make_unique<SmallVectorMemoryBuffer>(std::move(Buffer));
}    
......    
  Expected<std::unique_ptr<MemoryBuffer>> B =
      writeUniversalBinaryToBuffer(Slices);
  if (!B)
    return B.takeError();
  if (Error E = Out.allocate((*B)->getBufferSize()))
    return E;
  memcpy(Out.getBufferStart(), (*B)->getBufferStart(), (*B)->getBufferSize());  
  ^^^^^^^^^
  data is copied from memory into the output buffer

We could not use the memory-mapped file for writeUniversalBinaryToStream() since we do not know the final size.
Opposite, streams do not require knowing the final size so writeUniversalBinaryToStream() might write to the output
directly, without intermediate buffers.

That said, pretty sure LLVM's integrated assembler uses a pwrite stream to write out its object files, so if we can/if this patch does align with that method of output that seems OK.

That is one alternative which I suggest. We could use raw_pwrite_stream. If everybody agrees that it is correct choice I could update the patch accordingly.

But, initially, I suggested trying to use a, probably, better alternative - using raw_ostream.

So this patch as proposed is not using a pwrite stream, it seems?

yes. the patch in the current form suggests using raw_ostream. The reason for this is that raw_ostream is more general and has fewer restrictions
(it does not require to have seek/update functionality). It is good for the library to have a more general interface.

"if Writers could be implemented in such a way that data could be streamed without seeking/updating" - how does that work today, without the patch, and how does the patch address that need to seek?

without the patch, the data is already pre-allocated by external in-memory/mmap buffer:

llvm/tools/llvm-objcopy/ELF/Object.cpp

template <class ELFT> Error ELFWriter<ELFT>::finalize() {
...
  if (Error E = Buf.allocate(totalSize())) <<<<<<<<<<<<<<<<<<<<<<<
  return E;
...

So it is possible to seek into any place and update.

with this patch, an additional pre-allocated internal memory buffer is used. the data still could be randomly accessed.
When the writing is done then the whole buffer is written into the stream. The cost of that solution - an additional buffer.

Buf = WritableMemoryBuffer::getNewMemBuffer(totalSize());
...
// write ELF output to buf.
...
// TODO: Implement direct writing to the output stream
// TODO: (without intermediate memory buffer Buf).
Out.write(Buf->getBufferStart(), Buf->getBufferSize());
Out.flush();

I don't believe it's possible to write an ELF file without either knowing at least the sizes of all the sections up-front (you might be able to know these sizes without actually buffering the entire contents of the file) or seeking. Because the header needs to either contain an offset to the section table stored at the end of the file, where it knows the offsets/lengths of all the sections, or you write the section table at the start (so the header knows the offset up-front) and the table then needs to know the offsets of all the sections up-front.

I do not know ELF format/implementation well. But current implementation of ELF writer knows the size of the resulting file _before_ any writing started
(It allocates a buffer of the proper size). Since it knows the total output size, probably, it could pre-calculate offset to the section table for ELF header before writing started?

My initial suggestion is to try to use a more general interface(raw_ostream) and change it into the more specific(raw_pwrite_stream) if it would be inconvenient to use raw_ostream:

  1. Implement interfaces using raw_ostream:
Error executeObjcopyOnBinary(CopyConfig &Config,
                             object::Binary &In,
                             raw_ostream &Out);
  1. Use additional internal buffers to not to rewrite the writer's implementation.
  1. Someone(probably me, but later) changes the implementation of writers(ELF/COFF/MachO/Wasm) to not use internal buffers. So that writers store data into the output stream directly.
  1. If all implementations are successful - then leave raw_ostream in interfaces.
  1. If some implementations still require to seek/update functionality then change raw_ostream into raw_pwrite_stream:
Error executeObjcopyOnBinary(CopyConfig &Config,
                             object::Binary &In,
                             raw_pwrite_stream &Out);

If it is already clear that we need to use raw_pwrite_stream then it might be OK to use it in this patch from the start.

avl updated this revision to Diff 319312.Jan 26 2021, 8:50 AM

rebased.

We could not use the memory-mapped file for writeUniversalBinaryToStream() since we do not know the final size.

Is there some fundamental reason why you can't determine the final output size? In lld (wasm-ld at least) we go to some lengths to determine the final size before writing, basically for this reason. IIUC using a memory mapped output file has some big wins.

avl added a comment.EditedJan 27 2021, 7:30 AM

Is there some fundamental reason why you can't determine the final output size? In lld (wasm-ld at least) we go to some lengths to determine the final size before writing, basically for this reason.

I do not know whether exists some fundamental reason for not determining output size in the above case.
My comments based on the existing code:

Expected<std::unique_ptr<MemoryBuffer>> B =
    writeUniversalBinaryToBuffer(Slices);
if (!B)
  return B.takeError();
if (Error E = Out.allocate((*B)->getBufferSize()))
  return E;
memcpy(Out.getBufferStart(), (*B)->getBufferStart(), (*B)->getBufferSize());
return Out.commit();

Thus, I assumed that in that case, it is hard to calculate the final size.
Probably, the reason why the data is copied here is just not presenting class "Buffer" in the Support or Object libraries(So it could not be used inside the writeUniversalBinaryToBuffer()).

In the general case, the calculation of the final size might be the same expensive as the generation of the output file.
So it might be ineffective to pre-calculate the size.
That is why I am trying to suggest an interface that would not require buffer pre-allocation.

IIUC using a memory mapped output file has some big wins.

My suggestion is not about avoiding memory-mapped file usages.
I suggest an interface that allows to select/change the most useful data holder.
f.e. If we need to calculate SHA on the output of llvm-objcopy we would need
to store data in some temporary location and then do calculations:

MemBuffer/FileBuffer Out;
executeObjcopyOnBinary(Config, In, Out);
calculateSha(Out);

With the stream approach we do not need to allocate any temporary data at all:

raw_sha1_ostream Out;  <<< calculates SHA, do not allocate any data.
executeObjcopyOnBinary(Config, In, Out);

How memory-mapped files could be used with streams - (that solution suggested in https://reviews.llvm.org/D91693#2407437):

implement resizable mmap_ostream. mmap_ostream would allocate memory-mapped chunks.  If it needs to store more data it would allocate new chunks. Finally, it closes and concatenates all chunks into a single file.
if implementation uses reserve() method to inform mmap_ostream which size to preallocate then mmap_ostream would have the same performance as current FileOutputBuffer.

That solution allows to:

  1. select the most useful data holder.
  2. to use size precalculations only when it is cheap.
avl added a comment.Feb 15 2021, 4:51 AM

ping.

short summary why that refactoring is useful: It replaces specialized interfaces Buffer/MemBuffer/FileBuffer with more general interface - raw_ostream. It allows easily redefine the type of destination. It also allows to work effectively for implementations which do not pre-allocate destination space.

avl edited the summary of this revision. (Show Details)Feb 15 2021, 4:55 AM
dblaikie accepted this revision.Feb 20 2021, 11:12 AM
dblaikie added a subscriber: rnk.

Generally looks OK to me. Though would be good to have @jhenderson, @rupprecht, and maybe @rnk (for the COFF suport) take a look too.

(this is closer to how LLVM emits objects (it uses a raw_pwrite_stream, so it can go back to write the header since it doesn't compute the total layout/length up-front) but a bit further from how lld emits its output (it uses the llvm::FileOutputBuffer which is underneath the Buffer this patch moves away from) - which is a bit awkward, but at least maybe it's less of a 3rd way of doing things... maybe. Wolud be good to try to unify all these things some day)

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
133

Might be easier to review if this code was moved around in a separate preparatory commit. So the code changes would be separate from the code moving.

This revision is now accepted and ready to land.Feb 20 2021, 11:12 AM
jhenderson added inline comments.Feb 24 2021, 2:07 AM
llvm/tools/llvm-objcopy/COFF/Writer.cpp
389–393

You don't need TODO: on this second line. Also, your comment seems to have wrapped far earlier than the 80 character column limit, so you should reflow the comment. clang-format can reflow comments that are too long, as needed, although I don't think it does the inverse (i.e. reflow comments that are shorter than needed), so the easiest thing to do is just delete the new line and run clang-format on the comment.

392

Do we actually need this explicit flush? It seems to me like streams should flush before destruction, so there's no need to explicitly do it here, I'd think.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
740–741

@MaskRay deleted the build ID directory code in another patch (see https://reviews.llvm.org/D96310). We shouldn't be seeing that deletion in this patch too. Please rebase.

llvm/tools/llvm-objcopy/ELF/Object.cpp
2418

Same as above. Do we need the flush?

2558–2562

Same as above re. flush and TODO comments.

2648

Same as above re. flush.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
466

I'm not sure if it ever happened, but there was some discussion on the mailing lists about making 0 the default value if the size wasn't specified. Please could you check whether you need this second parameter still.

486

Same comment as above re. flush.

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp
517

If I'm not mistaken, this and equivalent messages elsewhere will just result in something like the following being printed:

error: 'foo.o': 42

You need to give the full description of the message (i.e. something like "failed to allocate memory buffer of XX bytes").

527–530

Same comment re. todo comments and flush.

llvm/tools/llvm-objcopy/Util.h
19–20 ↗(On Diff #319312)

Why is this function now in a new header file? Could it not be declared in one of the existing headers (like llvm-objcopy.h) and defined in the corresponding cpp?

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
133

+1 - this code move is unrelated. Please do it in a separate patch. Some of it might even be deletable after rebasing on D96310).

152–154

What's going on with this comment change? It looks like you've messed it up a bit.

I think what you want is "For regular files (as is the case for deepWriteArchive), FileOutputBuffer::create will return OnDiskBuffer."

242–245

Same comment as above.

347–350

I don't understand this change? This seems to be doing something quite different to what it was doing before.

llvm/tools/llvm-objcopy/wasm/Writer.cpp
79–82

Same comment as above re. flush and todo.

avl added a comment.Feb 24 2021, 9:15 AM

Will address all comments. I have a two questions/comments inside...

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
466

There is a comment for SmallVector that recommended practice is to not specify default value if there is no well motivated choice. Then it would automatically select one. In this case I think we do not need any stack data. So I specified 0 to avoid stack allocation, which looks correct in this case.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
347–350

It looks like it does the same. Originally, there were created a file called Config.OutputFilename and ProcessRaw put data in it. In this patch writeToFile helper creates a file called Config.OutputFilename and calls ProcessRaw to store data into it.

avl updated this revision to Diff 326456.Feb 25 2021, 11:43 AM

rebased. addressed comments.

avl added a comment.Mar 1 2021, 12:53 AM

@jhenderson Hi James, Is it OK now?

avl added a comment.Mar 2 2021, 5:37 AM

@rupprecht @rnk Do you have comments/objections for this refactoring?

Sorry, I've been unwell, and not been able to look more at this yet.

Also, one of our downstream users has recently run into a bug which probably impacts users of raw_fd_ostream, when writing from a Windows VM to a MacOS shared directory - they encounter "bad file descriptor" errors, in a manner simialr to https://bugs.llvm.org/show_bug.cgi?id=42623. I haven't filed a bug for this yet, as we're still trying to narrow down what is likely causing the problem, but I'm about 90% certain it is a general bug in raw_fd_ostream in this situation (copying to a memory mapped file as opposed to calling ::write works). At the moment, this bug affects llvm-objcopy when copying archives, but not objects, as the two wirting paths are different. It would be a shame if we introduced the bug into more areas, and I think this change does that. Perhaps we should try fixing that problem first?

avl added a comment.Mar 3 2021, 4:45 AM

Sorry, I've been unwell, and not been able to look more at this yet.

Also, one of our downstream users has recently run into a bug which probably impacts users of raw_fd_ostream, when writing from a Windows VM to a MacOS shared directory - they encounter "bad file descriptor" errors, in a manner simialr to https://bugs.llvm.org/show_bug.cgi?id=42623. I haven't filed a bug for this yet, as we're still trying to narrow down what is likely causing the problem, but I'm about 90% certain it is a general bug in raw_fd_ostream in this situation (copying to a memory mapped file as opposed to calling ::write works). At the moment, this bug affects llvm-objcopy when copying archives, but not objects, as the two wirting paths are different. It would be a shame if we introduced the bug into more areas, and I think this change does that. Perhaps we should try fixing that problem first?

I think yes it would be better to fix the problem first. Though we need a reproducer for that problem to not wait forever...

It looks like https://bugs.llvm.org/show_bug.cgi?id=42623 is already resolved in llvm by https://reviews.llvm.org/D81803. (But it has a problem with Windows7, which caused reverting in the rust compiler https://github.com/rust-lang/rust/issues/82677)

Please, let me known if I may help with fixing this.

In D91028#2599949, @avl wrote:

Sorry, I've been unwell, and not been able to look more at this yet.

Also, one of our downstream users has recently run into a bug which probably impacts users of raw_fd_ostream, when writing from a Windows VM to a MacOS shared directory - they encounter "bad file descriptor" errors, in a manner simialr to https://bugs.llvm.org/show_bug.cgi?id=42623. I haven't filed a bug for this yet, as we're still trying to narrow down what is likely causing the problem, but I'm about 90% certain it is a general bug in raw_fd_ostream in this situation (copying to a memory mapped file as opposed to calling ::write works). At the moment, this bug affects llvm-objcopy when copying archives, but not objects, as the two wirting paths are different. It would be a shame if we introduced the bug into more areas, and I think this change does that. Perhaps we should try fixing that problem first?

I think yes it would be better to fix the problem first. Though we need a reproducer for that problem to not wait forever...

It looks like https://bugs.llvm.org/show_bug.cgi?id=42623 is already resolved in llvm by https://reviews.llvm.org/D81803. (But it has a problem with Windows7, which caused reverting in the rust compiler https://github.com/rust-lang/rust/issues/82677)

Please, let me known if I may help with fixing this.

Ah, that might be good news. The user is using an LLVM 10 based toolchain, I believe, so wouldn't have this fix in. It's quite possible that when they upgrade to a later LLVM version, they will no longer see the problem. Unfortunately, the user is external to our company, and we don't currently have the setup to confirm the issue either way ourselves. However, by the sounds of it, if somebody does have a Windows 10 VM running using Parallels on a Mac, they should be able to reproduce by doing any generic archive copying with llvm-objcopy or writing using llvm-ar. If anybody on here has such a setup, it would be great if we can get that verification. LLVM 12 meanwhile hasn't yet been released, so I don't think it's practical (yet) for us to ask our user to try to confirm the fix for us.

If nobody does have the ability to reproduce this, I don't think we need to block this review on that, given the likely chance this has been fixed. I'll take a look at this patch again later today, assuming I get a moment.

In D91028#2599949, @avl wrote:

Sorry, I've been unwell, and not been able to look more at this yet.

Also, one of our downstream users has recently run into a bug which probably impacts users of raw_fd_ostream, when writing from a Windows VM to a MacOS shared directory - they encounter "bad file descriptor" errors, in a manner simialr to https://bugs.llvm.org/show_bug.cgi?id=42623. I haven't filed a bug for this yet, as we're still trying to narrow down what is likely causing the problem, but I'm about 90% certain it is a general bug in raw_fd_ostream in this situation (copying to a memory mapped file as opposed to calling ::write works). At the moment, this bug affects llvm-objcopy when copying archives, but not objects, as the two wirting paths are different. It would be a shame if we introduced the bug into more areas, and I think this change does that. Perhaps we should try fixing that problem first?

I think yes it would be better to fix the problem first. Though we need a reproducer for that problem to not wait forever...

It looks like https://bugs.llvm.org/show_bug.cgi?id=42623 is already resolved in llvm by https://reviews.llvm.org/D81803. (But it has a problem with Windows7, which caused reverting in the rust compiler https://github.com/rust-lang/rust/issues/82677)

Please, let me known if I may help with fixing this.

Ah, that might be good news. The user is using an LLVM 10 based toolchain, I believe, so wouldn't have this fix in. It's quite possible that when they upgrade to a later LLVM version, they will no longer see the problem. Unfortunately, the user is external to our company, and we don't currently have the setup to confirm the issue either way ourselves. However, by the sounds of it, if somebody does have a Windows 10 VM running using Parallels on a Mac, they should be able to reproduce by doing any generic archive copying with llvm-objcopy or writing using llvm-ar. If anybody on here has such a setup, it would be great if we can get that verification. LLVM 12 meanwhile hasn't yet been released, so I don't think it's practical (yet) for us to ask our user to try to confirm the fix for us.

If nobody does have the ability to reproduce this, I don't think we need to block this review on that, given the likely chance this has been fixed. I'll take a look at this patch again later today, assuming I get a moment.

Aside: can https://bugs.llvm.org/show_bug.cgi?id=42623 be resolved now?

jhenderson added inline comments.Mar 4 2021, 7:35 AM
llvm/tools/llvm-objcopy/COFF/Writer.cpp
389–392
llvm/tools/llvm-objcopy/ELF/Object.cpp
2417
2545

Elsewehere in this file, we don't qualify llvm::errc with the llvm::. I don't think you need it here either.

2547

LLVM coding standards state that errors shouldn't end in '.' - please delete the trailing one here.

2558–2561
2591–2593

As above. Remove the llvm:: qualifier from llvm::errc and delete the trailing full stop from the message.

2647
2697

Please write a more useful error message. This will result in a useless message of something like "error: foo.o: 0". This is the same issue as I already commented on in the mach-o area.

2713–2715

As above. Delete llvm:: and the trailing full stop.

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp
518–520

As above - no llvm:: or .

527
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
152–153
llvm/tools/llvm-objcopy/wasm/Writer.cpp
62–64
79
avl added a comment.Mar 4 2021, 7:57 AM

will address comments. as to checking the "Windows 10 VM running using Parallels on a Mac" configuration - unfortunately, I do not have that hardware/configuration. So I would not be able to check it in the nearest future.

avl updated this revision to Diff 328260.Mar 4 2021, 11:39 AM

adressed comments.

jhenderson accepted this revision.Mar 5 2021, 12:28 AM

LGTM, after the remaining nit is fixed. It might be worth trying to get somebody to confirm the discussed issue has been fixed. Perhaps worth asking on llvm-dev. A simple reproducible is to do something like "llvm-objcopy test.a path/on/shared/drive/test.a".

llvm/tools/llvm-objcopy/ELF/Object.cpp
2715
avl added a comment.Mar 8 2021, 12:46 PM

It might be worth trying to get somebody to confirm the discussed issue has been fixed. Perhaps worth asking on llvm-dev. A simple reproducible is to do something like "llvm-objcopy test.a path/on/shared/drive/test.a".

So far I managed to reproduce the problem in a bit different environment(Ubuntu host, Virtual Box VM, Windows guest). Reverting the https://reviews.llvm.org/D81803 patch restores the "LLVM ERROR: IO failure on output stream: bad file descriptor" problem. So I can confirm that https://reviews.llvm.org/D81803 cures the "bad file descriptor" problem. Though it seems has another problem with windows7(as it is already reported in https://reviews.llvm.org/D81803) and it might be reverted.

At the same time, it looks like this patch(https://reviews.llvm.org/D91028) does not change the status quo. The problem from https://reviews.llvm.org/D81803 is with TempFile::keep(). The TempFile::keep() is used by this patch for streams as well as it is used in the current mainline for OnDiskBuffer.commit(). i.e. the same problem arises with the current mainline and with this patch. Thus, I think, the problem with TempFile::keep() does not prevent integrating of this D91028.

In D91028#2612219, @avl wrote:

It might be worth trying to get somebody to confirm the discussed issue has been fixed. Perhaps worth asking on llvm-dev. A simple reproducible is to do something like "llvm-objcopy test.a path/on/shared/drive/test.a".

So far I managed to reproduce the problem in a bit different environment(Ubuntu host, Virtual Box VM, Windows guest). Reverting the https://reviews.llvm.org/D81803 patch restores the "LLVM ERROR: IO failure on output stream: bad file descriptor" problem. So I can confirm that https://reviews.llvm.org/D81803 cures the "bad file descriptor" problem. Though it seems has another problem with windows7(as it is already reported in https://reviews.llvm.org/D81803) and it might be reverted.

At the same time, it looks like this patch(https://reviews.llvm.org/D91028) does not change the status quo. The problem from https://reviews.llvm.org/D81803 is with TempFile::keep(). The TempFile::keep() is used by this patch for streams as well as it is used in the current mainline for OnDiskBuffer.commit(). i.e. the same problem arises with the current mainline and with this patch. Thus, I think, the problem with TempFile::keep() does not prevent integrating of this D91028.

The problem our downstream user has run into is seen only when using llvm-objcopy to copy archives, not regular objects, likely because it is only triggered by the writing via raw_fd_ostream, which is not done for regular object files. This change appears to extend the problem to object files too, if I'm not mistaken, hence my concern. However, if the issue is fixed, then it is not an issue either way.

FWIW, I don't think LLVM should be supporting runnning on Windows 7 anymore, since Windows 7 is now over a year past end of life, but that is a separate discussion.

avl added a comment.Mar 10 2021, 2:51 AM

The problem our downstream user has run into is seen only when using llvm-objcopy to copy archives, not regular objects, likely because it is only triggered by the writing via raw_fd_ostream, which is not done for regular object files. This change appears to extend the problem to object files too, if I'm not mistaken, hence my concern. However, if the issue is fixed, then it is not an issue either way.

Right. Looking at the fix for D81803 I had impression that only TempFile::keep() is affected. But after some debugging I see that ::write(from unistd.h) function(used by raw_fd_ostream) works incorrectly without the fix. Anyway D81803 seems cured the problem. thanks.

This revision was landed with ongoing or failed builds.Mar 10 2021, 12:52 PM
This revision was automatically updated to reflect the committed changes.
llvm/lib/Object/MachOUniversalWriter.cpp