Page MenuHomePhabricator

Make it possible to redirect not only errs() but also outs()
ClosedPublic

Authored by ruiu on Nov 14 2019, 11:10 PM.

Details

Summary

This patch adds a new parmeter to lld::*::link() so that we can pass
an raw_ostream object representing stdout. Previously, lld::*::link()
took only an stderr object.

Justification for making stdoutOS and stderrOS mandatory: I wanted to
make link() functions to take stdout and stderr in that order.
However, if we change the function signature from

bool link(ArrayRef<const char *> args, bool canExitEarly,
          raw_ostream &stderrOS = llvm::errs());

to

bool link(ArrayRef<const char *> args, bool canExitEarly,
          raw_ostream &stdoutOS = llvm::outs(),
          raw_ostream &stderrOS = llvm::errs());

, then the meaning of existing code that passes stderrOS silently
changes (stderrOS would be interpreted as stdoutOS). So, I chose to
make existing code not to compile, so that developers can fix their
code.

Diff Detail

Event Timeline

ruiu created this revision.Nov 14 2019, 11:10 PM
blackhole12 accepted this revision.Nov 14 2019, 11:50 PM

I can confirm this change would satisfy my use-case, although another reviewer ought to look over it.

This revision is now accepted and ready to land.Nov 14 2019, 11:50 PM
MaskRay accepted this revision.Nov 15 2019, 12:39 AM

The description should probably mention that this change is for people who use lld as a library (D70287). They call lld::*::link, instead of lld's main().

lld/lib/Driver/DarwinLdDriver.cpp
1147

I think we can just use stdoutOS and stderrOS, there is no need for consistency with local variable names. Note that args is also not capitalized...

ruiu marked an inline comment as done.Nov 15 2019, 12:53 AM
ruiu added inline comments.
lld/lib/Driver/DarwinLdDriver.cpp
1147

Yeah, I can use the variables directly, but I found that *stdoutOS << "something" look a bit awkward, and you can remove * if you make them functions.

This revision was automatically updated to reflect the committed changes.
jrtc27 added a subscriber: jrtc27.EditedNov 18 2019, 4:21 AM

This broke -DBUILD_SHARED_LIBS=ON for me; lld::errs() lives in Common, but Core has also been modified to use it, and Common depends on Core, not vice versa:

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:226 (lld/lib/Core/Resolver.cpp:226)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:255 (lld/lib/Core/Resolver.cpp:255)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:256 (lld/lib/Core/Resolver.cpp:256)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:269 (lld/lib/Core/Resolver.cpp:269)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:270 (lld/lib/Core/Resolver.cpp:270)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:282 (lld/lib/Core/Resolver.cpp:282)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:283 (lld/lib/Core/Resolver.cpp:283)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:427 (lld/lib/Core/Resolver.cpp:427)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::checkUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:434 (lld/lib/Core/Resolver.cpp:434)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::checkUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by SymbolTable.cpp:160 (lld/lib/Core/SymbolTable.cpp:160)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/SymbolTable.cpp.o:(lld::SymbolTable::addByName(lld::Atom const&))

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by SymbolTable.cpp:166 (lld/lib/Core/SymbolTable.cpp:166)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/SymbolTable.cpp.o:(lld::SymbolTable::addByName(lld::Atom const&))

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by SymbolTable.cpp:190 (lld/lib/Core/SymbolTable.cpp:190)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/SymbolTable.cpp.o:(lld::SymbolTable::addByName(lld::Atom const&))
collect2: error: ld returned 1 exit status

This broke -DBUILD_SHARED_LIBS=ON for me; lld::errs() lives in Common, but Core has also been modified to use it, and Common depends on Core, not vice versa:

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:226 (lld/lib/Core/Resolver.cpp:226)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:255 (lld/lib/Core/Resolver.cpp:255)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:256 (lld/lib/Core/Resolver.cpp:256)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:269 (lld/lib/Core/Resolver.cpp:269)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:270 (lld/lib/Core/Resolver.cpp:270)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:282 (lld/lib/Core/Resolver.cpp:282)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:283 (lld/lib/Core/Resolver.cpp:283)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::resolveUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:427 (lld/lib/Core/Resolver.cpp:427)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::checkUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by Resolver.cpp:434 (lld/lib/Core/Resolver.cpp:434)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/Resolver.cpp.o:(lld::Resolver::checkUndefines())

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by SymbolTable.cpp:160 (lld/lib/Core/SymbolTable.cpp:160)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/SymbolTable.cpp.o:(lld::SymbolTable::addByName(lld::Atom const&))

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by SymbolTable.cpp:166 (lld/lib/Core/SymbolTable.cpp:166)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/SymbolTable.cpp.o:(lld::SymbolTable::addByName(lld::Atom const&))

ld.lld: error: undefined symbol: lld::errs()
>>> referenced by SymbolTable.cpp:190 (lld/lib/Core/SymbolTable.cpp:190)
>>>               tools/lld/lib/Core/CMakeFiles/lldCore.dir/SymbolTable.cpp.o:(lld::SymbolTable::addByName(lld::Atom const&))
collect2: error: ld returned 1 exit status

My -DBUILD_SHARED_LIBS=ON build has the same problem. I think we need to move code around to fix this.

thopre added a subscriber: thopre.Nov 20 2019, 1:13 PM

I've raised PR44093 for a separate issue introduced by this commit. A tentative fix (that solve the problem for us) has been posted to phabricator.