This is an archive of the discontinued LLVM Phabricator instance.

Handle '-r' option properly
ClosedPublic

Authored by yunlian on Jun 5 2015, 10:39 AM.

Diff Detail

Event Timeline

yunlian updated this revision to Diff 27209.Jun 5 2015, 10:39 AM
yunlian retitled this revision from to Handle '-r' option properly.
yunlian updated this object.
yunlian edited the test plan for this revision. (Show Details)
yunlian added a reviewer: Bigcheese.
yunlian added a reviewer: rnk.Jun 5 2015, 11:03 AM
rnk accepted this revision.Jul 9 2015, 9:16 AM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jul 9 2015, 9:16 AM
yunlian closed this revision.Jul 10 2015, 1:14 PM
rnk added a comment.Jul 14 2015, 11:33 AM

This doesn't look like it got committed, did you want me to do that?

I have done arc commit --revision D<Revision> in my local svn repository and it seems to be successful, did I miss some thing?
If it does not get committed, please commit it for me, thanks.

rnk added a comment.Jul 14 2015, 3:36 PM

Hm, I tried to add a test for this while committing it, and it doesn't pass for me. I think this needs more work.

It seems this have not been checked in, could you please double check that?

Thanks

rnk added a comment.Jul 17 2015, 1:29 PM

It seems this have not been checked in, could you please double check that?

I tried to add a test for this while committing it, and it doesn't pass for me. I think this needs more work. Your 'NoArgumentUnused' flag suppresses the warning message without actually passing -r to the linker.

What is your test case?
I followed the bug https://llvm.org/bugs/show_bug.cgi?id=12587

Before my change, the output of 'file all.o' is
all.o: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, not stripped

After this change, the output is
all.o: ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped

So I think this patch is working.

rnk added a comment.Jul 20 2015, 12:40 PM

I tried adding -r to clang/test/Driver/Xlinker-args.c, and it did not get passed through.

Do you still have the test case?

yunlian updated this revision to Diff 33650.Aug 31 2015, 4:59 PM
yunlian edited edge metadata.

I uploaded a test case.
Without the patch, the test case fails, with the patch, the test case passes.

rnk added a comment.Sep 9 2015, 1:49 PM

I tried adding this test case, and it did not pass on Windows. I will apply it and run it again.

rnk added a comment.Sep 10 2015, 3:25 PM

Finally got my client clean and did this. Looks like it works, so I'll land it.