This is an archive of the discontinued LLVM Phabricator instance.

[ELF] PHDRS linker script command implemented.
AbandonedPublic

Authored by grimar on Feb 1 2016, 7:54 AM.

Details

Reviewers
ruiu
rafael
Summary

Patch implements PHDRS linker script command with few restrictions
which at this stage looks reasonable to live with.

Known issues/restrictions:

  • NONE header is not completely supported.
  • First header should be PT_PHDR, and first PT_LOAD should contain FILEHDR and PHDRS attribute.
  • Relro is currently disabled for linkerscript.

Main functionality is performed in createPhdrsFromScript() method.
It works like createPhdrs() but uses the options from linker script
to build headers map.

Diff Detail

Event Timeline

grimar updated this revision to Diff 46534.Feb 1 2016, 7:54 AM
grimar retitled this revision from to [ELF] PHDRS linker script command implemented..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
ruiu edited edge metadata.Feb 1 2016, 6:18 PM

Since this is a large patch, I applied this to my local repository to see how it would actually work, but it didn't compile (on Linux). Can you fix the build issue and upload it again?

In D16771#341324, @ruiu wrote:

Since this is a large patch, I applied this to my local repository to see how it would actually work, but it didn't compile (on Linux). Can you fix the build issue and upload it again?

Oh, yes, compiles fine using MSVS 2015, but fails on linux for me either, I will fix and reupload it. Sorry.

grimar updated this revision to Diff 46624.Feb 2 2016, 1:05 AM
grimar edited edge metadata.
  • Fixed compilation error and warning under linux.
grimar added a comment.Feb 2 2016, 6:17 AM
In D16771#341324, @ruiu wrote:

Since this is a large patch, I applied this to my local repository to see how it would actually work, but it didn't compile (on Linux). Can you fix the build issue and upload it again?

Btw, it is not that large. It seems large because has many crossing changes with http://reviews.llvm.org/D16575. If D16575 will be landed, most of them will be gone and I`ll update this patch then.

ruiu added a subscriber: ruiu.Feb 3 2016, 4:39 PM

Please give me a few more days to review this patch. Sorry for the delay.

In D16771#343647, @ruiu wrote:

Please give me a few more days to review this patch. Sorry for the delay.

That's fine. Thanks for your time in advance !

grimar added a comment.Feb 9 2016, 1:52 AM

I plan to update this basing on a Rafael's version of headers creation code (http://reviews.llvm.org/D16991).

grimar updated this revision to Diff 47797.Feb 12 2016, 6:17 AM
grimar updated this object.
  • Updated to match the latest changes in lld.
grimar added inline comments.Feb 12 2016, 6:23 AM
ELF/Writer.cpp
1209

Btw, is it supposed that createPhdrsFromScript() stuff should go to LinkerScript.cpp ?
Problem is that here the field of Writer is changed (Phdrs) and also methods and <ELFT> is used which I am not sure would look good in linkerscript files.

ruiu added inline comments.Feb 12 2016, 12:16 PM
ELF/LinkerScript.cpp
195–197

error() no longer call exit(), so you have to handle errors gracefully. In this case, if atEOF(), Token[Pos] is invalid.

358

Do not return an undefined value.

361

Needs a comment.

380

Handle errors gracefully.

ELF/Writer.cpp
1209

I think we probably want to move this to LinkerScript.cpp. Also this function is too long. Is there any way to reduce the amount of code and split into smaller pieces?

grimar added inline comments.Feb 14 2016, 10:56 PM
ELF/LinkerScript.cpp
195–197

yep, I know, just missed that place, sorry.

358

That was done intentionally. My point was that using of the return value in case of error is also a error (at least for that method), error flag should be checked, value can be accessedm but should not be used in any results because of invalid nature. Undefined value accepts to this requirements.
When you look the value of undefined variable it is usually clear it is the one, but defining it to something can hide the actual error.

380

Sorry, not sure I see something wrong here also. skip() has a check for error flag, so if something wrong here, it will just exit finally from both of while loops. There is no need of excessive error flags here and that is why I think it is gracefully already.

ELF/Writer.cpp
1209

I`ll try and update the patch later. Please clarify my comments above.

ruiu added inline comments.Feb 16 2016, 12:31 PM
ELF/LinkerScript.cpp
358

Returning an undefined variable is an undefined behavior, no? You cannot assume that an undefined value can be identified as such easily. In some compiler, it may, it totally depends on your environment. Since it is an undefined behavior, I think the compiler can even emit code to terminate the program at end of this function.

380

You need to check Error and exit from this loop. Currently I think this is an infinite loop if there is an opening '{' but no closing '}'.

rafael edited edge metadata.Feb 18 2016, 7:26 AM
rafael added a subscriber: rafael.

Do not return an undefined value.

That was done intentionally. My point was that using of the return value in case of error is also a error (at least for that method), error flag should be checked, value can be accessedm but should not be used in any results because of invalid nature. Undefined value accepts to this requirements.
When you look the value of undefined variable it is usually clear it is the one, but defining it to something can hide the actual error.

Returning an undefined variable is an undefined behavior, no?

Failing to return is. Not sure about returning an uninitialized
variable, but it does look odd.

Cheers,
Rafael

Do not return an undefined value.

That was done intentionally. My point was that using of the return value in case of error is also a error (at least for that method), error flag should be checked, value can be accessedm but should not be used in any results because of invalid nature. Undefined value accepts to this requirements.
When you look the value of undefined variable it is usually clear it is the one, but defining it to something can hide the actual error.

Returning an undefined variable is an undefined behavior, no?

Failing to return is. Not sure about returning an uninitialized
variable, but it does look odd.

Cheers,
Rafael

That will be fixed. It is really UB, I missed that.

grimar abandoned this revision.Jul 18 2016, 9:24 AM

That is pretty outdated and another version was posted by Eugene: D21995.