Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data flow: Fix a bad join order #18457

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Jan 9, 2025

#18333 follow-up.

While I did run DCA on the original PR, I did not rerun it on the last commit, because "what can possibly go wrong, right?". Turns out things did go wrong...

@hvitved hvitved added the no-change-note-required This PR does not need a change note label Jan 10, 2025
@hvitved hvitved marked this pull request as ready for review January 10, 2025 08:07
@Copilot Copilot bot review requested due to automatic review settings January 10, 2025 08:07

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll: Language not supported

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

@hvitved hvitved requested a review from aschackmull January 10, 2025 08:07
if allowParameterReturnInSelfEx(p)
then result.isNone()
else p.isParameterOf(_, result.asSome().(ParamUpdateReturnKind).getPosition())
}

bindingset[p]
pragma[inline_late]
private ReturnKindExtOption getDisallowedReturnKind(ParamNodeEx p) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going wrong? This predicate is binary, nomagic'ed and only used in two (nomagic'ed) contexts where the result isn't otherwise bound, so I fail to see what this transformation achieves?

Copy link
Contributor Author

@hvitved hvitved Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is needed because fwdFlowIn is inlined inside fwdFlowIsEntered. Without this change, each recursive iteration of fwdFlowIsEntered starts at getDisallowedReturnKind, instead of the delta. The DIL before is

noinline
nomagic
incremental
`DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowIsEntered/3#137febc2`(
  /* DataFlowDispatch::DataFlowCall */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCall call,
  /* Option::Option<DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::ReturnKindExt>::Option */ `Option#8eb11f23::Option<DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::ReturnKindExt>::TOption` disallowReturnKind,
  boolean cc
)
{
  [base_case] false()
  [recursive_case]
  exists(
    /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::ParamNodeEx */ `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` p
   |
    `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::getDisallowedReturnKind/1#a2c82816`(p,
      disallowReturnKind) and
    (
      (
        exists(
          /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` dummyField
         |
          not(
            `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::fullBarrier/1#e25deefa`(p)
          ) and
          `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::viableParamArgEx/3`(call,
            p, dummyField) and
          delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlow/2#65dc322b`(dummyField,
            cc)
        ) and
        (
          cc = false
          or
          (
            cc = true and
            not(
              `project#DataFlowImplCommon::Cached::CachedCallContextSensitivity::reducedViableImplInCallContext/3#f0f91b5d`(call)
            )
          )
        )
      )
      or
      (
        exists(
          /* DataFlowDispatch::DataFlowCallable */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCallable target
         |
          exists(
            /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ dontcare `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` _
           |
            previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowInReducedViableImplInSomeCallContext/4#ccb02962`(call,
              _, p, target)
          ) and
          delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::viableImplInSomeFwdFlowCallContextExt/1#23a13296`(call,
            target)
        ) and
        cc = true
      )
      or
      (
        exists(
          /* DataFlowDispatch::DataFlowCallable */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCallable target
         |
          exists(
            /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ dontcare `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` _
           |
            delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowInReducedViableImplInSomeCallContext/4#ccb02962`(call,
              _, p, target)
          ) and
          previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::viableImplInSomeFwdFlowCallContextExt/1#23a13296`(call,
            target)
        ) and
        cc = true
      )
    )
  ) and
  not(
    previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowIsEntered/3#137febc2`(call,
      disallowReturnKind, cc)
  )
}

and afterwards

noinline
nomagic
incremental
`DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowIsEntered/3#137febc2`(
  /* DataFlowDispatch::DataFlowCall */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCall call,
  /* Option::Option<DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::ReturnKindExt>::Option */ `Option#8eb11f23::Option<DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::ReturnKindExt>::TOption` disallowReturnKind,
  boolean cc
)
{
  [base_case] false()
  [recursive_case]
  exists(
    /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::ParamNodeEx */ `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` p
   |
    (
      (
        exists(
          /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` dummyField
         |
          delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlow/2#65dc322b`(dummyField,
            cc) and
          `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::viableParamArgEx/3`(call,
            p, dummyField)
        ) and
        (
          cc = false
          or
          (
            cc = true and
            not(
              `project#DataFlowImplCommon::Cached::CachedCallContextSensitivity::reducedViableImplInCallContext/3#f0f91b5d`(call)
            )
          )
        ) and
        not(
          `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::fullBarrier/1#e25deefa`(p)
        )
      )
      or
      exists(
        /* DataFlowDispatch::DataFlowCallable */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCallable target
       |
        cc = true and
        delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::viableImplInSomeFwdFlowCallContextExt/1#23a13296`(call,
          target) and
        exists(
          /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ dontcare `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` _
         |
          previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowInReducedViableImplInSomeCallContext/4#ccb02962`(call,
            _, p, target)
        )
      )
      or
      exists(
        /* DataFlowDispatch::DataFlowCallable */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCallable target
       |
        cc = true and
        exists(
          /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ dontcare `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` _
         |
          delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowInReducedViableImplInSomeCallContext/4#ccb02962`(call,
            _, p, target)
        ) and
        previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::viableImplInSomeFwdFlowCallContextExt/1#23a13296`(call,
          target)
      )
    ) and
    `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::getDisallowedReturnKind0/1#0f59df6c`(p,
      disallowReturnKind)
  ) and
  not(
    previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowIsEntered/3#137febc2`(call,
      disallowReturnKind, cc)
  )
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a pretty clear optimiser bug, I think: putting prev before delta like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually hadn't spotted that, only that getDisallowedReturnKind was pushed to the top, good spot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the actual reason why this join-order turns out poorly. The bindingset that we add in this PR isn't actually restricting the join-orderer in any way for the reason I stated, and that's what confused me - it's only giving a slight nudge that happens to cause the opimiser to dodge its bug.

@hvitved hvitved merged commit 039b2ec into github:main Jan 10, 2025
31 of 34 checks passed
@hvitved hvitved deleted the dataflow/disallowed-return-inline-late branch January 10, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DataFlow Library no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants