rtld: fix check for endianess of elf hints file

[tuexen]

Summary

Don't check if the elf hints file is in host byte order, but check if it is in little endian by looking at the magic number.

This fixes rtld on big endian platforms.

[se]

Thank you for finding the logic error.

The commented out extra condition was meant to be enabled in a future update to make little-endian systems assign a constant to is_le and to let constant expression evaluation effectively remove the conditional byte swap on them.

Since the code has now been tested and fixed on big-endian architectures, I'm going to enable that test (or add it back enabled, in case you have already removed it).

[tuexen]

In D44472#1014214, @se wrote:

Thank you for finding the logic error.

The commented out extra condition was meant to be enabled in a future update to make little-endian systems assign a constant to is_le and to let constant expression evaluation effectively remove the conditional byte swap on them.

My understanding is that is_le is true, iff the byte order of the hints file is little endian, not if the host byte order
is little endian. Is that correct?
Using le32toh(1) == 1 or better htole32(1) == 1 checks the host byte order, not the byte order of the file.

Using your condition would result in is_le being true if the host byte order is little endian or the file byte order
is little endian. So if you run this on a host with host byte order being little endian using a file with file byte order
being big endian, is_le would be true. Therefore you would use le32toh for the conversion, which wouldn't do
anything, and it would break.
So I think we can't use the condition I removed. Or the semantic of is_le is different... Or I'm misunderstanding
something else. Please let me know.

Since the code has now been tested and fixed on big-endian architectures, I'm going to enable that test (or add it back enabled, in case you have already removed it).

[se]

In D44472#1014219, @tuexen wrote:

In D44472#1014214, @se wrote:

Thank you for finding the logic error.

The commented out extra condition was meant to be enabled in a future update to make little-endian systems assign a constant to is_le and to let constant expression evaluation effectively remove the conditional byte swap on them.

My understanding is that is_le is true, iff the byte order of the hints file is little endian, not if the host byte order
is little endian. Is that correct?
Using le32toh(1) == 1 or better htole32(1) == 1 checks the host byte order, not the byte order of the file.

Using your condition would result in is_le being true if the host byte order is little endian or the file byte order
is little endian. So if you run this on a host with host byte order being little endian using a file with file byte order
being big endian, is_le would be true. Therefore you would use le32toh for the conversion, which wouldn't do
anything, and it would break.

Yes, the plan was to switch FreeBSD to always use a little-endian hints file on all architectures (i.e., all except powerpc64, currently).
This change can only be implemented when the pkg command has been updated to support either byte-order (i.e., after the next release of the pkg command - the code has already been committed to the pkg repository, based on elfhints.c from ldconfig, which did not have the issue fixed in this review).

So I think we can't use the condition I removed. Or the semantic of is_le is different... Or I'm misunderstanding
something else. Please let me know.

If we actually switch to little-endian hints files on all hosts (pending a change in ldconfig - see the comment in line 298 of ldconfig/elfhints.h), then there is no reason to support big-endian hints files on little-endian architectures.
As long as there are releases that use a native byte-order hints file on powerpc64 (AFAIK there is no other supported big-endian architecture left?), both byte-orders have to be supported on big-endian hosts.

Since the code has now been tested and fixed on big-endian architectures, I'm going to enable that test (or add it back enabled, in case you have already removed it).

I'd appreciate if you left the commented-out condition as-is, since I plan to enable it to remove the run-time penalty of compiling in this flexibility on little-endian machines.
They do not need it (except for my testing - I currently have a big-endian hints file on my amd64 machine), and I'd want this optimization to be activated in -CURRENT for the MFC to 14-STABLE before the next release is branched.

[tuexen]

In D44472#1014223, @se wrote:

In D44472#1014219, @tuexen wrote:

In D44472#1014214, @se wrote:

Thank you for finding the logic error.

The commented out extra condition was meant to be enabled in a future update to make little-endian systems assign a constant to is_le and to let constant expression evaluation effectively remove the conditional byte swap on them.

My understanding is that is_le is true, iff the byte order of the hints file is little endian, not if the host byte order
is little endian. Is that correct?
Using le32toh(1) == 1 or better htole32(1) == 1 checks the host byte order, not the byte order of the file.

Using your condition would result in is_le being true if the host byte order is little endian or the file byte order
is little endian. So if you run this on a host with host byte order being little endian using a file with file byte order
being big endian, is_le would be true. Therefore you would use le32toh for the conversion, which wouldn't do
anything, and it would break.

Yes, the plan was to switch FreeBSD to always use a little-endian hints file on all architectures (i.e., all except powerpc64, currently).

What is the plan?
(a) Switch the hints file to little endian on all platforms.
(b) Switch the hints file to little endian on all platforms except powerpc64.
Please clarify.

This change can only be implemented when the pkg command has been updated to support either byte-order (i.e., after the next release of the pkg command - the code has already been committed to the pkg repository, based on elfhints.c from ldconfig, which did not have the issue fixed in this review).

So I think we can't use the condition I removed. Or the semantic of is_le is different... Or I'm misunderstanding
something else. Please let me know.

If we actually switch to little-endian hints files on all hosts (pending a change in ldconfig - see the comment in line 298 of ldconfig/elfhints.h), then there is no reason to support big-endian hints files on little-endian architectures.

Sure. But why not just use the host byte order? I'm confused.

As long as there are releases that use a native byte-order hints file on powerpc64 (AFAIK there is no other supported big-endian architecture left?), both byte-orders have to be supported on big-endian hosts.

What about 32-bit PPC? Why would you use a hints file not in host byte order?

Since the code has now been tested and fixed on big-endian architectures, I'm going to enable that test (or add it back enabled, in case you have already removed it).

I'd appreciate if you left the commented-out condition as-is, since I plan to enable it to remove the run-time penalty of compiling in this flexibility on little-endian machines.
They do not need it (except for my testing - I currently have a big-endian hints file on my amd64 machine), and I'd want this optimization to be activated in -CURRENT for the MFC to 14-STABLE before the next release is branched.

[tuexen]

Add a comment describing the semantic of is_le and bring back a check for the host byte order.

[se]

Please commit with a relative short MFC delay (e.g. 1 week).
I had planned to MFC the (now fixed) code on March 26 (1 month after the commit to -CURRENT).
Since this fix is required to be merged with the initial commit, I plan to squash the cherry-picked commits (including your fix).

libexec/rtld-elf/rtld.c

2084

Seems there is a white-space issue ...

[tuexen]

libexec/rtld-elf/rtld.c

2084

It looks great locally. I guess this is a phabricator display issue.