fixed #13513 - adjusted access of known int values #7181

firewave · 2025-01-05T01:16:02Z

No description provided.

danmar

I like it.. good catch

firewave · 2025-01-06T12:26:10Z

This obviously still needs to use the proper calls but that might silently change the behavior and I would like to know if it does. But so far I have not been able to trigger any of those asserts and I do not feel comfortable putting those actually out there.

I will still give it a spin with daca and have some other tests to run which might help with finding at least a single test case.

firewave · 2025-01-18T15:15:12Z

I will take a tiered approach. Using the wrapping functions here and actually adjusting to the proper calls in a follow-up.

I am doing this because I still want to test if we might trigger this along with some other tests. And I also need to profile this to make sure that I do not accidentally introduce performance regressions. Also it is not a drop-in replacement in several cases.

firewave · 2025-01-18T16:18:09Z

I am doing this because I still want to test if we might trigger this along with some other tests.

Turns out the unit tests actually trigger this. I definitely ran those locally with the changes and had no failures. Not sure what I did wrong...

firewave · 2025-01-18T16:33:07Z

Turns out the unit tests actually trigger this. I definitely ran those locally with the changes and had no failures. Not sure what I did wrong...

The code in question was only added a few days ago. That explains why.

firewave · 2025-01-18T16:34:04Z

lib/symboldatabase.cpp

-            if (dimension_.tok && (dimension_.tok->hasKnownIntValue() ||
-                                   (dimension_.tok->isTemplateArg() && !dimension_.tok->values().empty()))) {
-                dimension_.num = dimension_.tok->getKnownIntValue();
+            if (dimension_.tok) {
+                if (dimension_.tok->hasKnownIntValue())
+                    dimension_.num = dimension_.tok->getKnownIntValue();
+                else if (dimension_.tok->isTemplateArg() && !dimension_.tok->values().empty()) {
+                    assert(dimension_.tok->values().front().isKnown());
+                    dimension_.num = dimension_.tok->values().front().intvalue;
+                }


@chrchr-github Please have a look. You changed this logic a few days ago in 46781df.

For template arguments, the values are only possible, not known. But since getKnownIntValue() just returns the first one, it is what we want in this case (I think).

So like the previous behavior? Just unconditionally use the first value and set dimension_.known to true?

Yes, pretty much. I imagine/hope that template arguments can have at most one value.

Maybe we should set those values to known in the future, and prevent unwanted knownConditionTrueFalse warnings in a different way.

I imagine/hope that template arguments can have at most one value.

I will just add a different assert.

pfultz2 · 2025-01-19T10:23:22Z

lib/valueflow.cpp

@@ -944,7 +944,7 @@ static void valueFlowRightShift(TokenList& tokenList, const Settings& settings)
        if (!tok->astOperand2()->hasKnownValue())
            continue;

-        const MathLib::bigint rhsvalue = tok->astOperand2()->values().front().intvalue;
+        const MathLib::bigint rhsvalue = tok->astOperand2()->getKnownValue()->intvalue;


Why not getKbownIntValue?

Because the preceding check is hasKnownValue().

That is a bug, it should be checking for a known int value since it's creating an int value below.

I see.

But the value is created on tok which has another hasKnownValue() check above. So that does not seem related (to me). Unless all of them need to be adjusted.

Also given the information in the other comment if this already has a non-int value and we set the int-value wouldn't that break that the case that it may only hold a single value (if that is even possible)?

It is related, all the checks in this pass should check for int value only.

pfultz2 · 2025-01-19T10:28:57Z

lib/token.cpp

+        return nullptr;
+    auto it = std::find_if(mImpl->mValues->begin(), mImpl->mValues->end(), std::mem_fn(&ValueFlow::Value::isKnown));
+    return it == mImpl->mValues->end() ? nullptr : &*it;
+}


There can be multiple known values of different types on a token and with the exception of int, they can be in any order. It seems like it would always be better to use the overload that takes the type.

There can be multiple known values of different types on a token and with the exception of int, they can be in any order.

Good to know. So I guess there is no need for an intermediate step and this can simply be cleaned up.

Could that assumption be used to simplify the lookup? A getKnownIntValue() which will bail out when the size is not 1 and it is not known? That would avoid unnecessary iterations. I will of course only do that if profiling shows that it would have an impact.

Should we add some kind of validation to make sure that it is always the case? A future change might accidentally break that.

It seems like it would always be better to use the overload that takes the type.

Yeah. I just aligned the code as a first step so the intent is clear and it is easier to refactor.

There can be multiple known values of different types on a token and with the exception of int, they can be in any order.

Good to know. So I guess there is no need for an intermediate step and this can simply be cleaned up.

What intermediate step?

Could that assumption be used to simplify the lookup? A getKnownIntValue() which will bail out when the size is not 1 and it is not known?

The size is not guaranteed to be 1. There could be other values, such as known from other types or possible values, etc.

For known int value you don't need iteration, you can just check the first element.

It seems like it would always be better to use the overload that takes the type.

Yeah. I just aligned the code as a first step so the intent is clear and it is easier to refactor.

The overload without a type doesn't express intent and very likely its a bug in the code.

firewave force-pushed the knownint branch 2 times, most recently from 3ac7fcc to 516a7f3 Compare January 5, 2025 12:23

danmar reviewed Jan 5, 2025

View reviewed changes

firewave changed the title ~~fixed #13513 - corrected access of known int values~~ refs #13513 - adjusted access of known int values Jan 18, 2025

firewave force-pushed the knownint branch from 516a7f3 to b4f0d28 Compare January 18, 2025 15:16

firewave force-pushed the knownint branch from b4f0d28 to e89c24a Compare January 18, 2025 16:32

firewave commented Jan 18, 2025

View reviewed changes

firewave force-pushed the knownint branch from e89c24a to 883aee6 Compare January 18, 2025 18:42

pfultz2 reviewed Jan 19, 2025

View reviewed changes

firewave force-pushed the knownint branch from 883aee6 to 7a7a8f3 Compare January 19, 2025 15:21

firewave added 5 commits January 19, 2025 17:39

refs #13513 - adjusted access of known int values

a44ba9a

symboldatabase.cpp: split logic in Variable::arrayDimensions()

738d875

Token: simplified hasKnownIntValue()

e08b026

checkcondition.cpp: added missing hasKnownIntValue() check

93910d3

valueflow.cpp: adjusted checks in valueFlowRightShift()

e5986ed

firewave changed the title ~~refs #13513 - adjusted access of known int values~~ fixed #13513 - adjusted access of known int values Jan 19, 2025

firewave force-pushed the knownint branch from 7a7a8f3 to e5986ed Compare January 19, 2025 16:46

firewave mentioned this pull request Jan 26, 2025

programmemory.cpp: avoid repeated iteration over values in Executor::executeImpl() #6701

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed #13513 - adjusted access of known int values #7181

fixed #13513 - adjusted access of known int values #7181

firewave commented Jan 5, 2025

danmar left a comment

firewave commented Jan 6, 2025

firewave commented Jan 18, 2025

firewave commented Jan 18, 2025

firewave commented Jan 18, 2025

firewave Jan 18, 2025

chrchr-github Jan 18, 2025

firewave Jan 18, 2025

chrchr-github Jan 18, 2025

chrchr-github Jan 18, 2025

firewave Jan 18, 2025

pfultz2 Jan 19, 2025

firewave Jan 19, 2025

pfultz2 Jan 19, 2025

firewave Jan 19, 2025

pfultz2 Jan 19, 2025

pfultz2 Jan 19, 2025

firewave Jan 19, 2025

pfultz2 Jan 19, 2025

fixed #13513 - adjusted access of known int values #7181

Are you sure you want to change the base?

fixed #13513 - adjusted access of known int values #7181

Conversation

firewave commented Jan 5, 2025

danmar left a comment

Choose a reason for hiding this comment

firewave commented Jan 6, 2025

firewave commented Jan 18, 2025

firewave commented Jan 18, 2025

firewave commented Jan 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment