This is an archive of the discontinued LLVM Phabricator instance.

[ELF2] - add support of --noinhibit-exec flag
ClosedPublic

Authored by grimar on Sep 24 2015, 9:34 AM.

Details

Summary

Support of --noinhibit-exec flag implemented.

Diff Detail

Event Timeline

grimar updated this revision to Diff 35639.Sep 24 2015, 9:34 AM
grimar retitled this revision from to [ELF2] - add support of --noinhibit-exec flag.
grimar updated this object.
grimar added reviewers: rafael, ruiu, rui314.
grimar added a project: lld.
grimar added subscribers: grimar, llvm-commits.

Requires the
void warning(const Twine &Msg)
method from dependency.

ruiu added inline comments.Sep 24 2015, 9:41 AM
ELF/Writer.cpp
4–6

"A Twine is not intended for use directly and should not be stored, its implementation relies on the ability to store pointers to temporary stack objects which may be deallocated at the end of a statement" (http://llvm.org/docs/doxygen/html/classllvm_1_1Twine.html#details)

So make this an std::string rather than a Tiwne.

grimar updated this revision to Diff 35648.Sep 24 2015, 10:06 AM

Review comments addressed.

ELF/Writer.cpp
288–290

Fixed. Sorry for that.

ruiu edited edge metadata.Sep 24 2015, 11:37 AM

BTW, is this guaranteed to be safe? I think we currently do not expect any undefined symbols to appear in Writer because we abort early if such symbol exists. This patch changes that assumption.

In COFF, we overwrite undefined symbols with absolute symbols with value 0 if /force (Windows option for -no-inhibit-exec). Maybe we should do that?

Rafael?

rafael edited edge metadata.Sep 24 2015, 2:17 PM

Just a few fixes.

This should be fine from the writer perspective since we are already handling weak undefined symbols.

ELF/Writer.cpp
288

I think you can write this as

std::string Error = "undefined symbol: " + Sym.getName();
if (SymFile)

Error += " in " ...
test/elf2/no-inhibit-exec.s
3

Also run llvm-objdump -d to check what we produce.

12

You don't need this part

grimar updated this revision to Diff 35712.Sep 25 2015, 3:19 AM
grimar edited edge metadata.

Review comments addressed

grimar marked 2 inline comments as done.Sep 25 2015, 3:21 AM
grimar added inline comments.
test/elf2/no-inhibit-exec.s
3

Done !

12

Agree. Fixed.

denis-protivensky added inline comments.
test/elf2/no-inhibit-exec.s
2

Maybe check that both runs issue the same message?

grimar added inline comments.Sep 25 2015, 5:39 AM
test/elf2/no-inhibit-exec.s
2

That might be useful. Can update the patch with that later.

rafael accepted this revision.Sep 25 2015, 6:57 AM
rafael edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 25 2015, 6:57 AM

I tried to apply it but failed:

git apply -p0 ~/Downloads/D13138.diff.txt
/Users/espindola/Downloads/D13138.diff.txt:9: trailing whitespace.

bool NoInhibitExec = false;

/Users/espindola/Downloads/D13138.diff.txt:21: trailing whitespace.

if (Args.hasArg(OPT_noinhibit_exec))

/Users/espindola/Downloads/D13138.diff.txt:22: trailing whitespace.

Config->NoInhibitExec = true;

/Users/espindola/Downloads/D13138.diff.txt:23: trailing whitespace.

/Users/espindola/Downloads/D13138.diff.txt:35: trailing whitespace.
#include "Config.h"
error: patch failed: ELF/Config.h:24
error: ELF/Config.h: patch does not apply

....

Can you email me a git generated patch directly?

Can you email me a git generated patch directly?

Just sent it.

grimar closed this revision.Sep 25 2015, 1:22 PM

Commited as r248605. I will improve the test separately.