[PATCH] D40558: [ELF] - Trigger error when -R <filename> is given.

[George Rimar via llvm-commits]

I think I agree that we should not make assumptions on runtime environment (so I would not touch current -rpath behavior).

Though my position here is based on safety mostly. It is definetely safe to leave current behavior which is consistent with GNU linkers

and is very simple (as does not require checking anything).


About -R I really do not know is it common or not (I thought not because we have no testcases for -R),

but assuming it is common and taking the fact it shares behavior

and LLD produces broken output silently, what about emiting warning like:

"-R (-rpath): option is depricated/unsupported. Consider using  -rpath or-just-symbols instead."

if -R was used ?


We are in good position to change behavior of badly designed things probably. And can do that in a hard or soft way,

warning is probably soft safe way. (And I believe it -R can be leaved as alias with this approach).


Best regards,
George | Developer | Access Softek, Inc
________________________________
От: Rui Ueyama <ruiu at google.com>
Отправлено: 30 ноября 2017 г. 4:30
Кому: Rafael Avila de Espindola
Копия: George Rimar; reviews+D40558+public+eb8f52c2c2ffee2b at reviews.llvm.org; Roland McGrath; Evgeny Leviant; llvm-commits; Ed Maste
Тема: Re: [PATCH] D40558: [ELF] - Trigger error when -R <filename> is given.

On Wed, Nov 29, 2017 at 8:59 AM, Rafael Avila de Espindola <rafael.espindola at gmail.com<mailto:rafael.espindola at gmail.com>> wrote:
George Rimar <grimar at accesssoftek.com<mailto:grimar at accesssoftek.com>> writes:

>>I think we shouldn't even emit an error for the second case. -rpath is the information for the dynamic linker, and the static linker should blindly store given -rpath >paths to a file header, because the static linker doesn't know the actual runtime environment. It is a valid use case that a given -rpath is not a directory in a build >environment but a directory in a runtime environment.
>>
>>Given that, I'm inclined to not to emit an error or a warning at all for -R and just make it an alias to -rpath. If you are so familiar with linker features that you want to >use --just-symbols, you are likely to know -R, -rpath and -just-symbols. -just-symbols is used very rarely, so in reality, I think this is not an issue.
>
> Two points from me:
> "Given that, I'm inclined to not to emit an error or a warning at all for -R and just make it an alias to -rpath. If you are so familiar with linker features that you want to use --just-symbols, you are likely to know -R"
>
> Last part means that we should implement -R both for -rpath and --just-symbols, like original GNU linkers do. As it is what is expected by user, who familar with both.
> Currently our behavior always behaves as -rpath without warnings or errors and produce broken output. And that nehavior is definetely not expected (and comments for PR35067 shows that).
>
> And then making -R an alias for one of these options is just not correct. If it's intention is to share 2 different behaviors it should be separate option with corresponding description, no ?
>
> ?Currently no our tests use -R for -rpath. What about if we drop supporting -R completely ? We can show a error suggesting to use explicitly use -rpath/--just-symbols instead.

I *think* that using -R for --rpath is fairly common.

I think I am back to favoring -R being an alias for rpath and producing
an error for "--rpath file" or "-R file". Yes, the build machine might
have a file and the intended run environment a directory, but that seems
unlikely and easy to avoid.

I'm not in favor of that. Reasons being that (1) assuming that a runtime environment is the same as a build environment is generally a bad idea even though it can be easily avoided, (2) --just-symbols is used very rarely, and (3) the -R option's behavior is undeniably a bit too hacky. Now seems like a good time to kill that tricky behavior.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171130/545d8965/attachment.html>