Page MenuHomePhabricator

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

Authored by rs on Jun 11 2015, 8:58 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

rs updated this revision to Diff 27510.Jun 11 2015, 8:58 AM
rs retitled this revision from to [ARM] Keep track of previous changes to the bit pattern stored HW_FP.
rs updated this object.
rs edited the test plan for this revision. (Show Details)
rs added a subscriber: Unknown Object (MLST).

Shouldn't this be fairly easily testable?

Tim.

rs added a comment.Jun 11 2015, 10:25 AM

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.

One minor comment.

lib/Basic/Targets.cpp
4274 ↗(On Diff #27510)

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

rs updated this revision to Diff 27658.Jun 15 2015, 3:22 AM

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

Thanks; I don't see any other problems.

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 added a comment.Jun 15 2015, 7:21 AM

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

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 added a comment.Jun 15 2015, 8:06 AM

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 added a comment.Jun 22 2015, 3:47 AM

can someone have another look at this?

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 added a comment.Jun 22 2015, 6:57 AM
// 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 added a comment.EditedJun 22 2015, 9:33 AM
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 added a comment.Jun 22 2015, 10:03 AM

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?

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 accepted this revision.Jun 24 2015, 3:27 PM
rengolin added a reviewer: 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. :)

This revision is now accepted and ready to land.Jun 24 2015, 3:27 PM
rs added a comment.Jun 24 2015, 4:40 PM

Thanks for the review.

This revision was automatically updated to reflect the committed changes.