Add module flags metadata to record the settings for enum and wchar width

[olista01]

Summary

Add module flags metadata to record the settings for enum and wchar width, to allow correct ARM build attribute generation

[rengolin]

Hi Oliver,

This code will force anyone not using the -f flags to have long enums and wchars. Is that true for *all* ARM architectures and instruction sets? The ABI document wasn't clear about this.

cheers,
--renato

[olista01]

This code will force anyone not using the -f flags to have long enums and wchars. Is that true for *all* ARM architectures and instruction sets? The ABI document wasn't clear about this.

I'm not sure I understand. The clang defaults on all of the architectures I tried are long enums and long wchar, so anyone not using the -f flags already has long enums and wchars. This just adds the build attributes, so that anyone using a linker which understands build attributes will not accidentally use the wrong libraries.

[compnerd]

wchar_t is unsigned long on any ARM environment strictly following AAPCS. Windows on ARM does not strictly follow AAPCS (it is pretty close though), and uses an unsigned short for wchar_t.

Enums are a bit more tricky, with them defaulting to short, but extending to long if any of the elements of the enumeration exceed the limits of short.

This seems to simply be recording the values for the short_wchar and short_enum, which is fine.

That said, would you be opposed to using an enumeration for the values of this metadata Oliver? That would allow us to extend the values in the future if we need to. If we do so, it would be nicer to rename the metadata to wchar_size and enum_size reflecting that this is not a boolean value. I realise that this would also require a change to the implementation in D4213.

[rengolin]

Even though we "assumed" that was true, we didn't have the attributes in place, if the default was different, say v4 from v7, than you'll get weird results. I'm just making sure this is intentional and clear.

But there's a second issue. This code will output the metadata on any architecture not just ARM, right? So, you'll get odd flags on IR where this has no meaning, and worse still, could be used in the future to mean something on a different architecture, which is not future proof.

I think a better approach is to *just* emit the metadata is the flag exists, and make the back-end work out what the defaults are on their own platforms and options.

[olista01]

wchar_t is unsigned long on any ARM environment strictly following AAPCS. Windows on ARM does not strictly follow AAPCS (it is pretty close though), and uses an unsigned short for wchar_t.

The AAPCS allows wchar_t to be unsigned int or unsigned short, with the choice left up to the platform ABI [1, 7.1.1]. Windows on ARM defines wchar_t to be unsigned short [2, C/C++ Specifics], which is allowed by the AAPCS.

Enums are a bit more tricky, with them defaulting to short, but extending to long if any of the elements of the enumeration exceed the limits of short.

I think we are getting our terminology mixed up. The two variants permitted by the AAPCS are [1, 7.1.3]:

• An enumerated type normally occupies a word (int or unsigned int). If a word cannot represent all of its enumerated values the type occupies a double word (long long or unsigned long long). This corresponds to clang's -fno-short-enums.
• The type of the storage container for an enumerated type is the smallest integer type that can contain all of its enumerated values. This corresponds to clang's -fshort-enums, which is the default.

According to the C standard (I only checked C99), the enum type is implementation defined [3, 6.7.2.2.4], but it looks like clang implements it as above for all targets (Sema::ActOnEnumBody, currently line 13014-13107).

That said, would you be opposed to using an enumeration for the values of this metadata Oliver?

With the current patch, the backend needs to know what the frontend is doing when using long/short wchar/enums. Perhaps it would be better to store the size of wchar_t and the minimum size of an enum? This should avoid the need to duplicate this target- and language-specific knowledge.

Even though we "assumed" that was true, we didn't have the attributes in place, if the default was different, say v4 from v7, than you'll get weird results.

If the default was different, the code will have been compiled with different assumptions about wchar_t and enum size, so we want the build attributes to prevent the user from linking these two objects together.

I think a better approach is to *just* emit the metadata is the flag exists.

With the current backend implementation, if there is no metadata the build attribute will not be emitted, and the default build attribute is that enums/wchars were not used as this is the only safe default. If the backend were to work out the default, this would duplicate the logic for selecting the default, and if we get it wrong the build attributes will not match the code.

1. http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042e/IHI0042E_aapcs.pdf
2. http://msdn.microsoft.com/en-us/library/dn736986.aspx
3. C99 draft with TC1-3

[olista01]

Emit the size of wchar_t and the minimum size of an enum, rather than a boolean representing which option was used.

[rengolin]

With the current backend implementation, if there is no metadata the build attribute will not be emitted, and the default build attribute is that enums/wchars were not used as this is the only safe default. If the backend were to work out the default, this would duplicate the logic for selecting the default, and if we get it wrong the build attributes will not match the code.

I know, and that was my point. I think we are in agreement as to what the dangers are (regarding dealing with the default and the current implementation, which might not be the same). I haven't gone through all possibilities myself, but I trust you have, so if you're happy with the metadata being applied for every ARM target, I'm happy too.

Though, it'd be best not to do that for any other target, since this is an ARM ABI specific flag.

[rengolin]

lib/CodeGen/CodeGenModule.cpp

364

two semi-colons

[olista01]

Only emit the metadata when targeting ARM.

[rengolin]

LGTM, thanks!

[olista01]

Thanks, committed revision 211354.

lib/CodeGen/CodeGenModule.cpp

getModule().addModuleFlag(llvm::Module::Warning, "Dwarf Version",
                               CodeGenOpts.DwarfVersion);
   if (DebugInfo)
     // We support a single version in the linked module. The LLVM
     // parser will drop debug info with a different version number
     // (and warn about it, too).
     getModule().addModuleFlag(llvm::Module::Warning, "Debug Info Version",
                               llvm::DEBUG_METADATA_VERSION);
 
+  // We need to record the widths of enums and wchar_t, so that we can generate
+  // the correct build attributes in the ARM backend.
+  llvm::Triple::ArchType Arch = Context.getTargetInfo().getTriple().getArch();
+  if (   Arch == llvm::Triple::arm
+      || Arch == llvm::Triple::armeb
+      || Arch == llvm::Triple::thumb
+      || Arch == llvm::Triple::thumbeb) {
+    // Width of wchar_t in bytes
+    uint64_t WCharWidth =
+        Context.getTypeSizeInChars(Context.getWideCharType()).getQuantity();
+    getModule().addModuleFlag(llvm::Module::Error, "wchar_size", WCharWidth);
+
+    // The minimum width of an enum in bytes
+    uint64_t EnumWidth = Context.getLangOpts().ShortEnums ? 1 : 4;
+    getModule().addModuleFlag(llvm::Module::Error, "min_enum_size", EnumWidth);
+  }
+
   SimplifyPersonality();
 
   if (getCodeGenOpts().EmitDeclMetadata)
     EmitDeclMetadata();
 
   if (getCodeGenOpts().EmitGcovArcs || getCodeGenOpts().EmitGcovNotes)
     EmitCoverageFile();
 

test/CodeGen/arm-metadata.c

+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -triple armv7a-linux-gnueabi -emit-llvm -o - %s | FileCheck -check-prefix=DEFAULT %s
+// RUN: %clang_cc1 -triple armv7a-linux-gnueabi -emit-llvm -o - %s -fshort-enums | FileCheck -check-prefix=SHORT-ENUM %s
+// RUN: %clang_cc1 -triple armv7a-linux-gnueabi -emit-llvm -o - %s -fshort-wchar | FileCheck -check-prefix=SHORT-WCHAR %s
+
+// DEFAULT:  !{{[0-9]+}} = metadata !{i32 1, metadata !"wchar_size", i32 4}
+// DEFAULT:   !{{[0-9]+}} = metadata !{i32 1, metadata !"min_enum_size", i32 4}
+
+// SHORT-WCHAR: !{{[0-9]+}} = metadata !{i32 1, metadata !"wchar_size", i32 2}
+// SHORT-WCHAR:   !{{[0-9]+}} = metadata !{i32 1, metadata !"min_enum_size", i32 4}
+
+// SHORT_ENUM:  !{{[0-9]+}} = metadata !{i32 1, metadata !"wchar_size", i32 4}
+// SHORT-ENUM:  !{{[0-9]+}} = metadata !{i32 1, metadata !"min_enum_size", i32 1}