This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Introduce --reproduce flag
ClosedPublic

Authored by davide on Apr 25 2016, 1:48 PM.

Details

Summary

Preface: lld is very likely to become the default linker in FreeBSD (in the next year or such).
In order to help the transition, we're going to ask FreeBSD developers to try out the new linker and report bugs.
--reproduce dumps the object files in a directory chosen (preserving the file system layout) and the linker invocation so that people can create an archive and upload to bugzilla.

Diff Detail

Event Timeline

davide updated this revision to Diff 54896.Apr 25 2016, 1:48 PM
davide retitled this revision from to [ELF] Introduce --reproduce flag.
davide updated this object.
davide added reviewers: rafael, emaste.
davide added a subscriber: llvm-commits.
ruiu added a subscriber: ruiu.Apr 25 2016, 2:05 PM
ruiu added inline comments.
ELF/Config.h
51

Remove = "". Strings are initialized to empty string by default. Also please rename Reproduce as the members of Config have the same name as command line options.

ELF/Driver.cpp
101–107

Why do you need to make a path full path?

115

I think you want to use path::append instead of appending "/".

118

Let's create a std::string for the directory name so that we don't need to repeat this string concatenation twice.

125

Let's make it explicit that the feature is optional.

if (Config->Reproduce)
  dumpFile(Path);
254
error(EC, "--reproduce: can't create directory");
261

auto -> const char *

262

This would leave a space character at end of the line and does not terminate the line with '\n'. As this is not a speed-critical path, I'd do

OS << llvm::join(Args.begin(), Args.end(), " ") << "\n";
283

Guard with if (Config->Reproduce) as well.

434–435

This error would be caught in dumpLinkerInvocation, no?

rafael edited edge metadata.Apr 25 2016, 2:32 PM
rafael added a subscriber: rafael.

For what it is worth, something like this would have been very handy
at creating the test directories I use for benchmarking :-)

davide updated this revision to Diff 54936.Apr 25 2016, 3:55 PM
davide edited edge metadata.
davide marked 8 inline comments as done.
davide added inline comments.
ELF/Driver.cpp
101–107

I don't think I need. Fixed.

262

I don't think your solution works because the way join works right now.
In fact, it doesnt' even build.

In file included from ../tools/lld/ELF/Driver.cpp:21:
../include/llvm/ADT/StringExtras.h:177:20: error: member reference base type 'const char' is not a structure or union

Len += (*Begin).size();
       ~~~~~~~~^~~~~

../include/llvm/ADT/StringExtras.h:192:10: note: in instantiation of function template specialization 'llvm::join_impl<const char *>' requested here

return join_impl(Begin, End, Separator, tag());
       ^

../tools/lld/ELF/Driver.cpp:271:15: note: in instantiation of function template specialization 'llvm::join<const char *>' requested here

OS << llvm::join(*Args.begin(), *Args.end(), " ");
            ^

1 error generated.

We might want to fix in StringExtras.h, but I'd rather do that later.
I added a '\n' at the end.

434–435

It doesn't seem so, i.e. if I remove the check:

[davide@localhost build-clang]$ rm -rf /home/davide/work/llvm/tools/lld/test/ELF/repro2
[davide@localhost build-clang]$ mkdir /home/davide/work/llvm/tools/lld/test/ELF/repro2
[davide@localhost build-clang]$ /home/davide/work/llvm/build-clang/./bin/not /home/davide/work/llvm/build-clang/./bin/ld.lld /home/davide/work/llvm/build-clang/tools/lld/test/ELF/Output/reproduce.s.tmp -o /home/davide/work/llvm/build-clang/tools/lld/test/ELF/Output/reproduce.s.tmp2 --reproduce /home/davide/work/llvm/tools/lld/test/ELF/repro2
[davide@localhost build-clang]$

ruiu added inline comments.Apr 25 2016, 4:03 PM
ELF/Driver.cpp
251

Remove trailing . and pass EC to error. Then error will format the message for you by appending a colon and EC.message().

error(EC, "--reproduce: can't create directory");
262

OK, but please do not emit trailing whitespace to the generated file.

431–432

Looks like create_directories accepts IgnoreExisting parameter as the second parameter.

std::error_code create_directories(const Twine &path,
                                   bool IgnoreExisting = true,
                                   perms Perms = owner_all | group_all);

You want to pass false then you can remove this code.

davide updated this revision to Diff 54945.Apr 25 2016, 4:51 PM

Addressed second round of comments.

ruiu added inline comments.Apr 25 2016, 4:58 PM
ELF/Driver.cpp
102–103

Remove.

105

Please do not add llvm:: for consistency.

106–107
if (std::error_code EC = sys::fs::create_directories(DirName))
108

Please pass EC as the first argument at this place too.

114–115
if (std::error_code EC = sys::fs::copy_file(SrcPath, DestPathName))
256

Please fix this place as well. You want to use sys::path::append instead of textually appending path fragments.

261

I'd do this (because ARGV[0] must exist)

OS << Args[0];
for (size_t I = 1, E = Args.size(); I < E; ++I)
  OS << " " << Args[I];
OS << "\n";
davide updated this revision to Diff 54948.Apr 25 2016, 5:07 PM
davide marked an inline comment as done.
ruiu added a comment.Apr 25 2016, 5:16 PM

LGTM with a nit.

ELF/Driver.cpp
429–430

Replace this with

Config->Reproduce = getString(Args, OPT_reproduce, "");

and move to the group at line 350-356 (and sort the group).

ruiu accepted this revision.Apr 25 2016, 5:16 PM
ruiu added a reviewer: ruiu.
This revision is now accepted and ready to land.Apr 25 2016, 5:16 PM
silvas added a subscriber: silvas.Apr 25 2016, 5:16 PM
silvas added inline comments.
ELF/Driver.cpp
263

An argument might contain a space, so let's separate these by newlines instead.
(an argument can technically contain a newline but I think that's sufficiently rare that we don't need to worry about that in a first patch.)

ruiu added inline comments.Apr 25 2016, 5:19 PM
ELF/Driver.cpp
263

Pathname with spaces are also rare (except on Windows), no? An upside of writing all options in one line is you can (re-)execute the command just by passing the file to the shell.

This revision was automatically updated to reflect the committed changes.
silvas added inline comments.Apr 25 2016, 5:29 PM
ELF/Driver.cpp
263

Pathname with spaces are also rare (except on Windows), no?

Exactly. Windows is one of our supported platforms.

An upside of writing all options in one line is you can (re-)execute the command just by passing the file to the shell.

For now, let's go ahead with how Davide had it (one line). We can make this more robust in future patches.