[ARM] Keep track of previous changes to the bit pattern stored HW_FP

[rs]

Summary

Variable HW_FP could get the wrong bit pattern based on how features are ordered in the Features vector. If a target had features '+neon' and '+fp-armv8' and the order of these features is ['+fp-armv8', '+neon'] then in the first iteration of the loop HW_FP would be set to the bit pattern 'HW_FP_SP | HW_FP_DP | HW_FP_HP' in the second iteration the value stored in HW_FP would be overwritten with 'HW_FP_SP | HW_FP_DP' and we would loose the bit HW_FP_HP from the previous iteration. The order of the features in the vector shouldn't change the final value in HW_FP.

[t.p.northover]

Shouldn't this be fairly easily testable?

Tim.

[rs]

TargetInfo::CreateTargetInfo takes the features from the command line and puts them into a StringMap and pulls them out again which can jumble the order of the features. So if I did write a test there's no guarantee that handleTargetFeatures will see them in the same order that I specified them on the command line. If you have any suggestions on how I can write a test for this then please let me know.

[scott-0]

One minor comment.

lib/Basic/Targets.cpp

4274

I think this would be more future proof as "HW_FP_remove |= ..."

[rs]

Changed 'HW_FP_remove =...' to 'HW_FP_remove |=...'

[scott-0]

Thanks; I don't see any other problems.

[rengolin]

In D10395#186872, @rs wrote:

TargetInfo::CreateTargetInfo takes the features from the command line and puts them into a StringMap and pulls them out again which can jumble the order of the features. So if I did write a test there's no guarantee that handleTargetFeatures will see them in the same order that I specified them on the command line. If you have any suggestions on how I can write a test for this then please let me know.

You can use CHECK-DAG, which will match all strings in a list in any order. Please, do write tests for this, it's quite easy to get that wrong.

cheers,
--renato

[rs]

The order that the features are specified on the cc1 command line won't mean that the features will be stored in the Features vector in the same order. This is because the features on the command line are parsed and stored into a StringMap then they're taken out again which can jumble up the order because StringMap's don't maintain order, below is the code which takes the features out of the StringMap in lib/Basic/Targets.cpp:

7311:  Opts->Features.clear();
7312:  for (llvm::StringMap<bool>::const_iterator it = Features.begin(),
7313:         ie = Features.end(); it != ie; ++it)
7314:    Opts->Features.push_back((it->second ? "+" : "-") + it->first().str());
7315:  if (!Target->handleTargetFeatures(Opts->Features, Diags))
7316:    return nullptr;

7318:  return Target.release();

You can use CHECK-DAG, which will match all strings in a list in any order

Can you explain in more detail how I can check for this internal bug using CHECK-DAG?

Thanks,
Ranjeet

[rengolin]

In D10395#187995, @rs wrote:

Can you explain in more detail how I can check for this internal bug using CHECK-DAG?

I'm assuming this is the target features, that normally come in the format:

+foo,-bar,+baz

or

-target-feature +foo -target-feature -bar -target-feature +baz

Either case, if you use:

CHECK-DAG: +foo
CHECK-DAG: -bar
CHECK-DAG: +baz

They could come in any order and it would still match.

[rs]

Can you explain in more detail how I can check for this internal bug using CHECK-DAG?

I'm assuming this is the target features, that normally come in the format:

+foo,-bar,+baz

or

-target-feature +foo -target-feature -bar -target-feature +baz

Either case, if you use:

CHECK-DAG: +foo
CHECK-DAG: -bar
CHECK-DAG: +baz

They could come in any order and it would still match.

Do you mean matching against the -cc1 command line? There's no way for a test case to determine in which order the features come out of the StringMap and are processed. Changing the order on the -cc1 command line may still process the features in the same order. So I don't see how to write a test case that ensures the old code fails.

[rs]

can someone have another look at this?

[rengolin]

In D10395#188008, @rs wrote:

Do you mean matching against the -cc1 command line?

Yes.

There's no way for a test case to determine in which order the features come out of the StringMap and are processed.

That's perfectly fine.

So I don't see how to write a test case that ensures the old code fails.

// RUN: echo "clang -cc1 -target-feature +foo -target-feature -bar -target-feature +baz" | FileCheck %s
// RUN: echo "clang -cc1 -target-feature +baz -target-feature -bar -target-feature +foo" | FileCheck %s
// RUN: echo "clang -cc1 -target-feature +foo -target-feature +baz -target-feature -bar" | FileCheck %s

// CHECK-DAG: -target-feature +foo
// CHECK-DAG: -target-feature -bar
// CHECK-DAG: -target-feature +baz

[rs]

// RUN: echo "clang -cc1 -target-feature +foo -target-feature -bar -target-feature +baz" | FileCheck %s
// RUN: echo "clang -cc1 -target-feature +baz -target-feature -bar -target-feature +foo" | FileCheck %s
// RUN: echo "clang -cc1 -target-feature +foo -target-feature +baz -target-feature -bar" | FileCheck %s

// CHECK-DAG: -target-feature +foo
// CHECK-DAG: -target-feature -bar
// CHECK-DAG: -target-feature +baz

I don't think this will ensure the old code fails.

If I could write a test case for this bug I would probably write it like this:

// RUN: echo "clang -cc1 -target-feature +foo -target-feature -bar -target-feature +baz" | FileCheck %s
// RUN: echo "clang -cc1 -target-feature +baz -target-feature -bar -target-feature +foo" | FileCheck %s
// RUN: echo "clang -cc1 -target-feature +foo -target-feature +baz -target-feature -bar" | FileCheck %s

// CHECK: _ARM_FP <correct value>

however I can't write a test case and I'll explain why.

// RUN: echo "clang -cc1 -target-feature +foo -target-feature -bar -target-feature +baz" | FileCheck %s
// RUN: echo "clang -cc1 -target-feature +baz -target-feature -bar -target-feature +foo" | FileCheck %s
// RUN: echo "clang -cc1 -target-feature +foo -target-feature +baz -target-feature -bar" | FileCheck %s

In the test case the target features are listed in different orders ['+foo', '-bar', '+baz'], ['+baz', '-bar', '+foo'] and ['+foo', '+baz', '-bar']. These features are parsed and put into a StringMap that maps the feature strings to a boolean value ('+' or '-'), so in our example the StringMap will look like:

'foo' -> true
'bar' -> false
'baz' -> true

later in the code, the code below is executed. It pulls the features out of the StringMap and puts them into a vector which is then passed to the method handleTargetFeatures which is where the bug with the variable 'HW_FP' exists. If the order you get things out of a StringMap is guaranteed to be the same order that things are put in then yes you could use those 3 test cases but it's not, you could get one permutation that triggers the bug you can get another that won't trigger the bug, it's possible that the 3 orders in the test example are all pulled out of the StringMap in the same order. The StringMap is an intermediate container for the features in between the features getting parsed from the command line and them getting pulled out and put into a vector to be passed to handleTargetFeatures, if the container was something that guarantees order then yes you could use the test cases but it's not.

// Add the features to the compile options.
//
// FIXME: If we are completely confident that we have the right set, we only
// need to pass the minuses.
Opts->Features.clear();
for (llvm::StringMap<bool>::const_iterator it = Features.begin(),
       ie = Features.end(); it != ie; ++it)
  Opts->Features.push_back((it->second ? "+" : "-") + it->first().str());
if (!Target->handleTargetFeatures(Opts->Features, Diags))
  return nullptr;

[rengolin]

In D10395#191658, @rs wrote:

// Add the features to the compile options.
//
// FIXME: If we are completely confident that we have the right set, we only
// need to pass the minuses.
Opts->Features.clear();
for (llvm::StringMap<bool>::const_iterator it = Features.begin(),
       ie = Features.end(); it != ie; ++it)
  Opts->Features.push_back((it->second ? "+" : "-") + it->first().str());
if (!Target->handleTargetFeatures(Opts->Features, Diags))
  return nullptr;

Why are you testing for _ARM_FP if the core of what you're doing is adding +/- strings to -target-features? Why not test the target-features directly?

It doesn't really matter that your StringMap is non-deterministic, if all +stuff and -stuff appear in any order, you're good.

I'm not trying to make you reproduce the old bug if it is non-deterministic (that would be folly), I'm just saying that you should test that the expected target features are coming from the right options.

If you need to test _ARM_FP, that's a different, simpler test: just put a bunch of variations and see that all of them give what you expect.

If there are already tests that do that (both target features and __ARM_FP, then just point to them and that's fine).

cheers,
--renato

[rs]

Why are you testing for _ARM_FP if the core of what you're doing is adding +/- strings to -target-features? Why not test the target-features directly?

Because it's set to the value of HW_FP and HW_FP is what could get the wrong value in the previous code.

// ACLE 6.5.1 Hardware Floating Point
if (HW_FP)
  Builder.defineMacro("__ARM_FP", "0x" + llvm::utohexstr(HW_FP));

If you need to test _ARM_FP, that's a different, simpler test: just put a bunch of variations and see that all of them give what you expect.

There already exists a test that checks what the value of _ARM_FP is, it's in clang/test/Preprocessor/arm-acle-6.5.c.

I'm just saying that you should test that the expected target features are coming from the right options.

I don't mind adding a test for this but it seems unrelated to the bug I'm trying to fix in this patch, could you explain a bit more as to why you want this test?

[rengolin]

In D10395#191786, @rs wrote:

I don't mind adding a test for this but it seems unrelated to the bug I'm trying to fix in this patch, could you explain a bit more as to why you want this test?

There was a behaviour that prompted you to implement this fix: The macro wasn't defined in some cases. You changed the behaviour of an FP-selection routine, and all tests still pass. This means two things:

1. The tests weren't good enough to cover for the case you saw because...
2. The wrong behaviour you're fixing is not easy to test.

The tests are not meant to always exactly reproduce the behaviour, but if you can get an internal behaviour tested which will reflect in the macro being set, then that's a good way to make sure the macro will never be broken again. That's the very essence of unit testing.

My suggestion that you could check the target features is that, in a way, they will reflect that doesn't matter the order in which you put them in, the FP macro will be set. It doesn't have to have the macro in it at all, just the justification that, with the corner cases covered, the macro will be set.

[rengolin]

We had a chat at the pub, (as any gentlemen would), and now I realise why it is hard to do the tests that we really need here. The target-info ones would be at least bogus, if not redundant, so yes, please commit. LGTM.

Thanks! And sorry for the trouble. :)

[rs]

Thanks for the review.

lib/Basic/Targets.cpp

public:
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override {
     FPU = 0;
     CRC = 0;
     Crypto = 0;
     SoftFloat = SoftFloatABI = false;
     HWDiv = 0;
 
+    // This does not diagnose illegal cases like having both
+    // "+vfpv2" and "+vfpv3" or having "+neon" and "+fp-only-sp".
+    uint32_t HW_FP_remove = 0;
     for (const auto &Feature : Features) {
       if (Feature == "+soft-float") {
         SoftFloat = true;
       } else if (Feature == "+soft-float-abi") {
         SoftFloatABI = true;
       } else if (Feature == "+vfp2") {
         FPU |= VFP2FPU;
-        HW_FP = HW_FP_SP | HW_FP_DP;
+        HW_FP |= HW_FP_SP | HW_FP_DP;
       } else if (Feature == "+vfp3") {
         FPU |= VFP3FPU;
-        HW_FP = HW_FP_SP | HW_FP_DP;
+        HW_FP |= HW_FP_SP | HW_FP_DP;
       } else if (Feature == "+vfp4") {
         FPU |= VFP4FPU;
-        HW_FP = HW_FP_SP | HW_FP_DP | HW_FP_HP;
+        HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
       } else if (Feature == "+fp-armv8") {
         FPU |= FPARMV8;
-        HW_FP = HW_FP_SP | HW_FP_DP | HW_FP_HP;
+        HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
       } else if (Feature == "+neon") {
         FPU |= NeonFPU;
-        HW_FP = HW_FP_SP | HW_FP_DP;
+        HW_FP |= HW_FP_SP | HW_FP_DP;
       } else if (Feature == "+hwdiv") {
         HWDiv |= HWDivThumb;
       } else if (Feature == "+hwdiv-arm") {
         HWDiv |= HWDivARM;
       } else if (Feature == "+crc") {
         CRC = 1;
       } else if (Feature == "+crypto") {
         Crypto = 1;
       } else if (Feature == "+fp-only-sp") {
-        HW_FP &= ~HW_FP_DP;
+        HW_FP_remove |= HW_FP_DP | HW_FP_HP;
       }
     }
+    HW_FP &= ~HW_FP_remove;
 
     if (!(FPU & NeonFPU) && FPMath == FP_Neon) {
       Diags.Report(diag::err_target_unsupported_fpmath) << "neon";
       return false;
     }
 
     if (FPMath == FP_Neon)
       Features.push_back("+neonfp");