Page MenuHomePhabricator

[ARM][AArch64] Pass through endianness flags to the GNU assembler and linker
ClosedPublic

Authored by peter.smith on Oct 2 2018, 8:04 AM.

Details

Summary

The big-endian arm32 Linux builds are currently failing when the -mbig-endian and -fno-use-integrated-as flags are used and the assembler default is little endian. We are not registering that the clang -mbig-endian flag needs to be passed through to the assembler to override its default target. This also extends to the linker, where the presence of -mbig-endian or -mlittle-endian can affect the emulation and the output endianness.

This patch explicitly passes -EB or -EL to the linker and assembler to override the default endianness of the assembler and linker.

For Arm, the -mbig-endian and -mlittle-endian do not affect the Architecture in Triple::computeTargetTriple(), see Triple::getBigEndianArchVariant() and Triple::getLittleEndianArchVariant() for details. This means we need to account for it when
determining what linker emulation to use and which endian flag to pass to the assembler and linker. For AArch64 (and other targets) Triple::computeTargetTriple() will account for -mbig-endian and -mlittle-endian and alter the Arch field of the triple.

Fixes pr38770

FIXME: While fixing this I note that giving the endianness in march does not override the endianness in the triple. This is not specific to -fno-integrated-as so if I write --target=arm-linux-gnueabihf -march=armebv7-a then the output is little endian. I've updated the targets in the test so that the target endian matches the march endianness.

I think that this behaviour is wrong for a couple of reasons:

  • -march=armebv7-a should either give an error message if it disagrees with the triple or should override the triple.
  • The GNU assembler doesn't understand eb in march so we should ideally not pass it through, or rely on people to not use march that way.

This can be fixed but I've not done so in this patch.

Diff Detail

Repository
rC Clang

Event Timeline

peter.smith created this revision.Oct 2 2018, 8:04 AM

Hi Peter,

Looks ok. Why did you need to change all cmdlines to have -EL? I imagine you just need one for each case, everything else remains the default (which should still work).

Also, it would be interesting to know what happens on cases like "aarch64_be -EL" et al, and have negative tests for them.

cheers,
--renato

Hi Peter,

Looks ok. Why did you need to change all cmdlines to have -EL? I imagine you just need one for each case, everything else remains the default (which should still work).

I thought that in general we wouldn't know what the default endianness of the assembler we are targeting is so it was safest to be explicit. For a cross-toolchain it is probably inherent in the name, but in theory we could be running clang on a native big-endian system where the assembler is just called as.

Also, it would be interesting to know what happens on cases like "aarch64_be -EL" et al, and have negative tests for them.

Ok will add some more tests.

cheers,
--renato

Updated diff to add more tests, including some that use -mlittle-endian when the target is big-endian.

Ping. Does anyone have any changes they'd like me to make?

I think we should pass these flags along to the linker, too. Not just the assembler. Maybe within tools::gnutools::Linker::ConstructJob() within lib/Driver/ToolChains/Gnu.cpp?

lib/Driver/ToolChains/Gnu.cpp
572–578

If you passed llvm::Triple instead of bool, then I think you could do something like:

static const char* GetEndianArg(const llvm::Triple &triple, const ArgList &Args) {
  const bool IsBigEndian = triple == llvm::Triple::armeb ||
                           triple == llvm::Triple::thumbeb ||
                           triple == llvm::Triple::aarch64_be ||
                           Args.getLastArg(options::OPT_mlittle_endian,
                                           options::OPT_mbig_endian)->getOption().matches(options::OPT_mbig_endian);
  return IsBigEndian ? "-EB" : "-EL";
}

Might encapsulate the logic from the call sites better, but that's a minor nit.

573–576

Just so I understand this check, even if we didn't have a BE triple, we set IsBigEndian if -mlittle-endian was not set? Is -mlittle-endian a default set flag, or does this set "-EB" even if no -m*-endian flag is specified (I think we want little-endian to be the default, and IIUC, this would change that)?

Thanks for the comments. I agree with you that the -EB and -EL flags need to be passed to the linker as well, I kind of expected that ld.bfd would infer it from the endianness of the first object but it doesn't seem to do that. If it's ok I'll do that in a separate patch?

I hope I've been able to explain the test. I'm on the fence about passing in the triple as a parameter, I have a mild preference for the way it is but if you'd like me to change it I can.

lib/Driver/ToolChains/Gnu.cpp
572–578

I did think about separating it from the triple. In the end I thought it best to follow the existing switch on the triple and put up with a bit of duplication.

As it stands I think the above wouldn't quite work as we could have --target=armeb-linux-gnueabi -mlittle-endian which would get short-circuited to big endian if all the tests are done at once.

I think it would be possible to do something like:

bool IsBigEndian = getTriple().getArch() == llvm::Triple::armeb ||
                                 getTriple().getArch() == llvm::Triple::thumbeb ||
                                 getTriple().getArch() == llvm::Triple::aarch64_be;
if (Arg *A = Args.getLastArg(options::OPT_mlittle_endian, options::OPT_mbig_endian))
      IsBigEndian = !A->getOption().matches(options::OPT_mlittle_endian);
return IsBigEndian ? "-EB" : "-EL";

but we'd only want to call it for Arm and AArch64 so I don't think it saves us much.

573–576

I stole the logic from ToolChain::ComputeLLVMTriple in ToolChain.cpp.

On entry to the function IsBigEndian will be set to true if the triple is one of armeb, thumbeb or aarch64eb. Otherwise it will be false.

The getLastArg(options::OPT_mlittle_endian, options_mbig_endian) will return NULL if neither was set, so we'd default to the value in the triple.

On reflection after looking at what would be needed for the linker tools::gnutools::Linker::ConstructJob() I think it may be better to move the triple detection into a separate function. I'll work on that and will hopefully post an update soon.

peter.smith retitled this revision from [ARM][AArch64] Pass through endianness flags to the GNU assembler to [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker.Oct 12 2018, 6:16 AM
peter.smith edited the summary of this revision. (Show Details)

I've decided to roll the linker changes in with the assembler ones as the linker use case affects the design. It turns out that only Arm needs to check to see if the -mbig-endian and -mlittle-endian flags override the triple as computeTargetTriple() accounts for them for AArch64.

I renamed arm::appendEBLinkFlags to arm::appendBE8LinkFlag as it exclusively adds "--be8" for Armv7 and above targets.

Thanks for the addition of the flags to the linker. Interesting note about -m*-endian only being applicable for armv7. Just some minor nits left.

With the current version of the patch, I can now assemble, link, and boot in a virtualized environment a big-endian armv8 Linux kernel with Clang. :)

lib/Driver/ToolChains/Gnu.cpp
231

// On Arm and endianness

drop first and

268–269

would a ?: ternary fit on one line here?

return isArmBigEndian(T, Args) ? "armelfb_linux_eabi" : "armelf_linux_eabi";

363–370
bool IsBigEndian = isArmBigEndian(Triple, Args);
if (IsBigEndian)
  arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
IsBigEndian |= Arch == llvm::Triple::aarch64_be;
CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
368

is having the else if on its own line what the formatter chose?

670–673

Can we fit a ternary in one line here as well?

CmdArgs.push_back(isArmBigEndian(Triple2, Args) ? "-EB" : "-EL");
703

earlier (L362), you check the endianess of the triple with:

Arch == llvm::Triple::aarch64_be

where Arch is ToolChain.getArch().

I don't have a preference, but these two seem inconsistent. Can we either check the explicit llvm::Triple:: or call getToolChain().getTriple().isLittleEndian() in both, rather than mix?

lib/Driver/ToolChains/Gnu.cpp
363–370

IsBigEndian |= Arch == llvm::Triple::aarch64_be;

should be:

IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;

in order to not evaluate Arch == llvm::Triple::aarch64_b if IsBigEndian is already true.

peter.smith marked 7 inline comments as done.Oct 15 2018, 5:42 AM

Thanks very much for the comments. I'll post an update shortly.

lib/Driver/ToolChains/Gnu.cpp
363–370

Thanks for the suggestion. One thing it highlighted was that isArmBigEndian could return true for an aarch64_be arch with -mbig-endian so I've rewritten isArmBigEndian to always return false if the architecture isn't Arm and have added some test cases to check that "--be8" doesn't sneak in.

368

I'd forgot to run clang-format over that part of the code. I've adopted the snippet below which replaces it.

703

I originally took that part from the Mips code, I've replaced it with a check against aarch64_be which is more consistent with the other Arm and AArch64 code.

peter.smith marked 3 inline comments as done.

Updated diff to reflect review comments. Main changes are:

  • isArmBigEndian always returns false if the target architecture isn't Arm.
  • Added tests to make sure "--be8" doesn't get added by mistake (would have been in previous patch for aarch64_be arch with -mbig-endian flag.
nickdesaulniers accepted this revision.Oct 15 2018, 11:15 AM

Thanks for this patch. With it I was able to link+boot a BE aarch64 Linux kernel (and a LE aarch64 Linux kernel).

lib/Driver/ToolChains/Gnu.cpp
268

probably don't need the parens around isArmBigEndian(...).

This revision is now accepted and ready to land.Oct 15 2018, 11:15 AM
This revision was automatically updated to reflect the committed changes.

Thanks again for this patch!

xbolva00 added inline comments.
lib/Driver/ToolChains/Gnu.cpp
241

Gnu.cpp:241:17: warning: this statement may fall through [-Wimplicit-fallthrough=]

IsBigEndian = true;

Thanks for pointing that out, I'm out of office today, will look at describing the intention to fall through when I get back in on Monday.

lib/Driver/ToolChains/Gnu.cpp
241

The fall through is intentional in this case.

xbolva00 added inline comments.Oct 19 2018, 12:17 AM
lib/Driver/ToolChains/Gnu.cpp
241

Please mark it with LLVM_FALLTHROUGH then.

Added LLVM_FALLTHROUGH; in r344890.