[ARM] Set the thumb flag for all text segments on COFF (partial revert of recent commit)

[mstorsjo]

Summary

This is a partial revert of changes in SVN rev 273880 (changes that are unrelated to what is described in the commit message).

That commit also changed MCObjectFileInfo to only set the thumb flag if the triple architecture is thumb, no longer setting it if the architecture is arm. This is probably correct per se, but negatively affects standalone assembly files assembled via clang for windows.

When assembling files via clang, even if the target triple set on the command line is e.g. thumb-win32, the driver sets the triple to armv7--windows-msvc18.0.0 (or similar; see tools/clang/lib/Driver/ToolChain.cpp) since assembly files should start in ARM mode by default.

This means that it currently is impossible to get the thumb code flag set for these sections, when assembling via clang.

[rengolin]

When assembling files via clang, even if the target triple set on the command line is e.g. thumb-win32, the driver sets the triple to armv7--windows-msvc18.0.0 (or similar; see tools/clang/lib/Driver/ToolChain.cpp) since assembly files should start in ARM mode by default.

This makes no sense. If this is an assembly file, it should have some Thumb markers (.code 16, .thumb_func) to be classified as Thumb. You can't just assume it's Thumb because "COFF is probably Windows".

[mstorsjo]

In D22776#495277, @rengolin wrote:

When assembling files via clang, even if the target triple set on the command line is e.g. thumb-win32, the driver sets the triple to armv7--windows-msvc18.0.0 (or similar; see tools/clang/lib/Driver/ToolChain.cpp) since assembly files should start in ARM mode by default.

This makes no sense. If this is an assembly file, it should have some Thumb markers (.code 16, .thumb_func) to be classified as Thumb.

True (and the code I'm working with obviously has such markers as well). Even if files have Thumb markers, the code currently just looks for the triplet (which was set at the start of the session), disregarding whatever mode currently is used.

You can't just assume it's Thumb because "COFF is probably Windows".

True, although that assumption is pretty widespread so far (which of course isn't a reason to spread it further).

Prior to SVN rev 273880, such files assembled correctly at least (even if the non-existent non-thumb ARM Windows assembly didn't).

How much harder is a proper fix for this issue? (I'm quite unfamiliar with the LLVM codebase so far, and unfortunately don't have much time to spare. So in case it's a much larger scope, I'm afraid I'll have to defer this to a bug report.)

[rengolin]

In D22776#495298, @mstorsjo wrote:

True (and the code I'm working with obviously has such markers as well). Even if files have Thumb markers, the code currently just looks for the triplet (which was set at the start of the session), disregarding whatever mode currently is used.

Right, this sounds terribly wrong. :)

Unfortunately, assuming Thumb even when market ARM is not the solution. What if someone actually chose -marm with COFF?

How much harder is a proper fix for this issue? (I'm quite unfamiliar with the LLVM codebase so far, and unfortunately don't have much time to spare. So in case it's a much larger scope, I'm afraid I'll have to defer this to a bug report.)

That's a good question. I'm unfamiliar with COFF and Windows as well. I'm adding some folks to chime in.

[compnerd]

We actually do not support non-Windows COFF AFAIK. We have many places where we assert that this is the case. Windows ARM is a pure thumb-2 environment, so its a relatively safe assumption to make. Please add a test case so that we don't accidentally get into this situation again.

[rengolin]

In D22776#495638, @compnerd wrote:

We actually do not support non-Windows COFF AFAIK. We have many places where we assert that this is the case. Windows ARM is a pure thumb-2 environment, so its a relatively safe assumption to make.

It may be a safe assumption, but that's bad code. If that's always true, than whatever that flag was representing is no longer relevant.

My point is that the behaviour is broken. Martin said even with thumb markers the code is still coming up as ARM, and that's wrong. If we fix that, we can still have the flag as it is.

[mstorsjo]

Added a test for this issue. Whichever way it is solved, this test should pass.

[rengolin]

The only "quick fix" that I'd find acceptable here would be something along the lines of:

bool isWinARM = (T.getOS() == Triple::Windows &&
                (T.getArch() == Triple::arm || T.getArch() == Triple::thumb));

then replace "isThumb" by "isWinARM" and add a big FIXME saying Windows on ARM Triples are broken.

But I'd be much happier with a proper fix to the Triple on ARM/Windows.

lib/MC/MCObjectFileInfo.cpp

604

I'm opposed to adding this line at all.

AFAICS, this is only initialising the global attributes. On Linux, a file can have ARM and Thumb functions and support both of them. COFF is largely Windows but it's not *just* Windows, for example, EFI code is COFF and doesn't run on Windows.

If Windows is always Thumb, no matter what, then the Triple *must* identify that. If the Triple doesn't, than this is a bug in the front-end and it should not have to be hacked in the back-end, completely ignoring the front-end decisions.

test/MC/ARM/Windows/thumb-attributes.s

19 ↗

(On Diff #65700)

This is fragile, as the order and structure can change with unrelated code changes.

I'd just CHECK for the ones you need, using CHECK-DAG, so that they can change in order. (spaces are irrelevant on CHECKs).

[mstorsjo]

In D22776#497392, @rengolin wrote:

The only "quick fix" that I'd find acceptable here would be something along the lines of:

bool isWinARM = (T.getOS() == Triple::Windows &&
                (T.getArch() == Triple::arm || T.getArch() == Triple::thumb));

then replace "isThumb" by "isWinARM" and add a big FIXME saying Windows on ARM Triples are broken.

For correctness, shouldn't you keep setting the flag also for non-windows thumb? I.e. something like this:

bool isWinARM = (T.getOS() == Triple::Windows &&
                (T.getArch() == Triple::arm || T.getArch() == Triple::thumb));
bool isThumb = T.getArch() == Triple::thumb;
[...]
    ((isThumb || isWinARM) ? COFF::IMAGE_SCN_MEM_16BIT : (COFF::SectionCharacteristics)0)

But I'd be much happier with a proper fix to the Triple on ARM/Windows.

The proper fix isn't so much about triples I think, as about tracking the thumb/arm mode flag and propagating it here. Those flags are per-function - what if there are mixed thumb/arm functions in a single text section (when this flag here is set per section)? Ignoring the contrieved case of mixed mode functions, you'd still need to look up e.g. the first function in the segment, and I'm not sure if that's available at all at this point.

As for why the triples are wrong, see lines 487-503 at http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChain.cpp?revision=276848&view=markup - this is not about windows per se, but the clang driver always passes an arm triple to the assembler, regardless of what is set on the command line (even for all other OSes). The issue is that COFF needs to know this per-section, and triples seem to be the only thing easily available (and there, windows-coff implies thumb).

[mstorsjo]

Updated the patch with changes requested before. Alternatively, D22855 can be used to fix the same issue in the clang driver instead.

[rengolin]

In D22776#497465, @mstorsjo wrote:

Updated the patch with changes requested before. Alternatively, D22855 can be used to fix the same issue in the clang driver instead.

You Clang fix is much better, thank you! Feel free to abandon this review.

[mstorsjo]

Fixed by D22855 instead.

lib/MC/MCObjectFileInfo.cpp

void MCObjectFileInfo::initCOFFMCObjectFileInfo(const Triple &T) {
   EHFrameSection = Ctx->getCOFFSection(
       ".eh_frame", COFF::IMAGE_SCN_CNT_INITIALIZED_DATA |
                        COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_MEM_WRITE,
       SectionKind::getData());
 
   // Set the `IMAGE_SCN_MEM_16BIT` flag when compiling for thumb mode.  This is
   // used to indicate to the linker that the text segment contains thumb instructions
   // and to set the ISA selection bit for calls accordingly.
-  const bool IsThumb = T.getArch() == Triple::thumb;
+  const bool IsThumb = T.getArch() == Triple::arm || T.getArch() == Triple::thumb;
 
   CommDirectiveSupportsAlignment = true;
 
   // COFF
   BSSSection = Ctx->getCOFFSection(
       ".bss", COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA |
                   COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_MEM_WRITE,
       SectionKind::getBSS());
   TextSection = Ctx->getCOFFSection(
       ".text",
       (IsThumb ? COFF::IMAGE_SCN_MEM_16BIT : (COFF::SectionCharacteristics)0) |