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

[Renato Golin via llvm-commits]

rengolin requested changes to this revision.
rengolin added a reviewer: rengolin.
rengolin added a comment.
This revision now requires changes to proceed.

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.


================
Comment at: lib/MC/MCObjectFileInfo.cpp:604
@@ -602,1 +603,3 @@
+  // is set to arm-* since the assembler always should start in ARM mode.
+  const bool IsThumb = T.getArch() == Triple::arm || T.getArch() == Triple::thumb;
 
----------------
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.

================
Comment at: test/MC/ARM/Windows/thumb-attributes.s:19
@@ +18,3 @@
+@ CHECK:       IMAGE_SCN_ALIGN_4BYTES
+@ CHECK:       IMAGE_SCN_CNT_CODE
+@ CHECK:       IMAGE_SCN_MEM_16BIT
----------------
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).


https://reviews.llvm.org/D22776