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

[Ranjeet Singh]

>   // 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;




http://reviews.llvm.org/D10395

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/