Page MenuHomePhabricator

[ELF] --warn-backrefs: use the same GroupId for object files in the same --{start,end}-lib
ClosedPublic

Authored by MaskRay on Apr 19 2018, 4:39 PM.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

MaskRay created this revision.Apr 19 2018, 4:39 PM
ruiu added a comment.Apr 19 2018, 4:48 PM

Does --start-lib/--end-lib really implies --start-group/--end-group?

t3.o .globl bar; bar:
t4.o .globl foo; foo: call bar
t.o .global _start; _start: call bar

ld.gold t.o --start-lib t3.o t4.o --end-lib # successfully

Without this revision, ld.lld --warn-backrefs t.o --start-lib t3.o t4.o --end-lib warns that t4.o has a reference on t3.o.

ruiu added a comment.Apr 19 2018, 4:57 PM

If that's the case, --start-lib implies --start-group. To handle it, I'd set InputFile::IsInGroup to true for --start-lib just like we are doing for --start-group, so that --start-lib is handled as if --start-group everywhere.

MaskRay updated this revision to Diff 143198.Apr 19 2018, 5:10 PM

Treat --start-lib as --start-group when using --warn-backrefs

Updated.

gold disallows --start-group --start-lib nested in any order so we can simplify the handling.

// gold/options.cc

void
Input_arguments::start_group()
{
  if (this->in_group_)
    gold_fatal(_("May not nest groups"));
  if (this->in_lib_)
    gold_fatal(_("may not nest groups in libraries"));
  Input_file_group* group = new Input_file_group();
  this->input_argument_list_.push_back(Input_argument(group));
  this->in_group_ = true;
}

// Start a lib.

void
Input_arguments::start_lib(const Position_dependent_options& options)
{
  if (this->in_lib_)
    gold_fatal(_("may not nest libraries"));
  if (this->in_group_)
    gold_fatal(_("may not nest libraries in groups"));
  Input_file_lib* lib = new Input_file_lib(options);
  this->input_argument_list_.push_back(Input_argument(lib));
  this->in_lib_ = true;
}
MaskRay updated this revision to Diff 143200.Apr 19 2018, 5:28 PM

Better error

MaskRay updated this revision to Diff 143202.Apr 19 2018, 5:33 PM

Update test

ruiu added inline comments.Apr 19 2018, 5:53 PM
ELF/Driver.cpp
987

Do you need this? I think you now always set IsInGroup if you are InLib, so this new condition doesn't seem necessary.

Does --start-lib/--end-lib really implies --start-group/--end-group?

Not exactly. --start-lib/--end-lib means that those files are treated like a single .a. The tradition ELF handling of a .a file will loop over the members until no new member is fetched.

MaskRay updated this revision to Diff 143216.Apr 19 2018, 7:09 PM

if (!InputFile::IsInGroup || InLib)
->
if (!InputFile::IsInGroup)

MaskRay marked an inline comment as done.Apr 19 2018, 7:09 PM
This revision is now accepted and ready to land.Apr 20 2018, 8:03 AM
This revision was automatically updated to reflect the committed changes.

This revision breaks build systems where the current directory isn't writable, because the test warn-backrefs.s just writes to a.out.

I have committed R330464 to add an output argument, which fixes the test.