Page MenuHomePhabricator

[ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values
AbandonedPublic

Authored by labrinea on Nov 22 2016, 5:40 AM.

Details

Summary

The ABI for the ARM architecture describes four build attribute values for the size of enumeration types. Currently, Clang cannot instruct LLVM to emit the value 0 (use of enums is prohibited) and 3 (every enumeration visible across an ABI-complying interface contains a value needing 32 bits to encode). This patch adds the Driver flags '-fno-enums' and '-fabi-enums' to indicate the missing values. The build attribute values are passed to LLVM via metadata in the IR.

This patch depends on Emit the missing Tag_ABI_enum_size build attribute values.

Diff Detail

Event Timeline

labrinea updated this revision to Diff 78851.Nov 22 2016, 5:40 AM
labrinea retitled this revision from to [ARM] Add Driver support for emmitting the missing Tag_ABI_enum_size build attribute values.
labrinea updated this object.
labrinea added reviewers: cfe-commits, rengolin.
labrinea updated this object.Nov 22 2016, 5:41 AM
labrinea retitled this revision from [ARM] Add Driver support for emmitting the missing Tag_ABI_enum_size build attribute values to [ARM] Add Driver support for emitting the missing Tag_ABI_enum_size build attribute values.Nov 22 2016, 5:46 AM
labrinea updated this object.Nov 22 2016, 5:52 AM
labrinea updated this object.
rengolin edited edge metadata.Nov 22 2016, 6:08 AM

Hi Alexandros,

My interpretation of Tag_ABI_enum_size is that value 3 is that all enums are 32-bit and value 4 is that only those ABI-visible (ie. public interfaces) have to be 32-bits. This is similar to the other PCS tags.

So, the table is:

  • 0: no enums
  • 1: short enums
  • 2: all 32-bit
  • 3: public 32-bit

With the default being 2 in GCC.

My reading of your code here is that you assume short|abi = short, which doesn't match the table in the ABI.

lib/Basic/Targets.cpp
5407

Isn't ABIEnums 4? Shouldn't this be:

Opts.ShortEnums || !Opts.ABIEnums

Is it even valid to have ABIEnums && ShortEnums at the same time?

lib/Driver/Tools.cpp
5731

If this is true, why go the extra complexity of mapping all possible states? Why not just use one line:

Enum = Args.getLastArg(options::OPT_fno_enums, options::OPT_fshort_enums, options::OPT_fabi_enums);

and be done with it? Short and ABI are not compatible, it's either one or the other.

5741

This code is confusing and merging with each other. Please add some empty lines between them.

labrinea added inline comments.Nov 22 2016, 6:29 AM
lib/Basic/Targets.cpp
5407

My understanding is that ABIEnums requires 32-bit enums across an ABI-complying interface, but allows short enums outside of it. Therefore __ARM_SIZEOF_MINIMAL_ENUM could be any of 1 or 4.

ABIEnums && ShortEnums cannot be both set at the same time.

lib/Driver/Tools.cpp
5731

I don't like this complicated logic either but it can handle cases like:
-fshort-enums -fno-enums -fabi-enums -fno-abi-enums
where -fno-enums should win. I've added a test for this sequence (test/Driver/clang_f_opts.c, line 459)
The suggested logic would ignore '-fno-abi-enums', which is the last argument. If we are happy with rejecting all the preceding flags and keeping just the last one this would work:

Args.getLastArg(options::OPT_fno_enums, options::OPT_fshort_enums, options::OPT_fno_short_enums,
                options::OPT_fabi_enums, options::OPT_fno_abi_enums);

Alternatively an equally ugly logic that handles complicated sequences is:

if (Arg *Enum = Args.getLastArg(options::OPT_fno_enums,
             ShortEnums ? options::OPT_fshort_enums : options::OPT_INVALID,
               ABIEnums ? options::OPT_fabi_enums   : options::OPT_INVALID)) {
rengolin added inline comments.Nov 22 2016, 6:56 AM
lib/Basic/Targets.cpp
5407

If all four options are mutually exclusive, than they should be a single integer option, not multiple boolean ones.

lib/Driver/Tools.cpp
5731

I disagree. These are different flags controlling the same thing, so the last-option wins.

How does -ffast-math vs its internal options are handled? How is -O handled? Or the other ABI flags like vfp, hard-float?

Easier still, have a look at unsigned/signed char below. Last arg wins.

How does GCC behave with those arguments?

Thinking back now, we may be required to follow, as the principle of least surprise has to hold.

Your suggestion that if all four options are mutually exclusive, then they should be a single integer option, totally makes sense to me. We could use a single integer option and make the old boolean flags deprecated (i.e. map '-fshort-enums' and 'fno-short enums' to the new integer option). My concern is that we have to be GCC compatible. I will communicate this to the GNU community.

labrinea abandoned this revision.Dec 6 2016, 2:24 AM

Hi Renato, apologies for the long silence. Unfortunately this work is more complicated than I initially thought. We'll have to rethink about it thoroughly. I am going to abandon the patch for now. Thank you for reviewing this.