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

[BPF] improve error handling by custom lowering & fail() #75088

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

inclyc
Copy link
Member

@inclyc inclyc commented Dec 11, 2023

Currently on mcpu=v3 we do not support sdiv, srem instructions. And the backend crashes with stacktrace & coredump, which is misleading for end users, as this is not a "bug"

Add llvm bug reporting for sdiv/srem on ISel legalize-op phase.

For clang frontend we can get detailed location & bug report.

$ build/bin/clang -g -target bpf -c local/sdiv.c
local/sdiv.c:1:35: error: unsupported signed division, please convert to unsigned div/mod.
    1 | int sdiv(int a, int b) { return a / b; }
      |                                   ^
1 error generated.

Fixes: #70433
Fixes: #48647

This also improves error handling for dynamic stack allocation:

local/vla.c:2:3: error: unsupported dynamic stack allocation
    2 |   int b[n];
      |   ^
1 error generated.

Fixes: #57171

@inclyc
Copy link
Member Author

inclyc commented Dec 12, 2023

Also CC @4ast

Currently on mcpu=v3 we do not support sdiv, srem instructions.
And the backend crashes with stacktrace & coredump, which is misleading
for end users, as this is not a "bug"

Add llvm bug reporting for sdiv/srem on ISel legalize-op phase.

For clang frontend we can get detailed location & bug report.

    $ build/bin/clang -g -target bpf -c local/sdiv.c
    local/sdiv.c:1:35: error: unsupported signed division, please convert to unsigned div/mod.
        1 | int sdiv(int a, int b) { return a / b; }
          |                                   ^
    1 error generated.

Fixes: #70433
Fixes: #48647

This also improves error handling for dynamic stack allocation:

    local/vla.c:2:3: error: unsupported dynamic stack allocation
        2 |   int b[n];
          |   ^
    1 error generated.

Fixes: #57171
@inclyc inclyc force-pushed the users/inclyc/bpf-dont-crash-sdiv-srem branch from 1cc86c2 to 63661ff Compare December 12, 2023 10:10
@inclyc inclyc changed the title [BPF] improve error handling for unsupported sdiv, srem [BPF] improve error handling by custom lowering & fail() Dec 12, 2023
@eddyz87
Copy link
Contributor

eddyz87 commented Dec 12, 2023

Hi @inclyc, thank you for working on this.

I think these chages are fine.
The the default Expand action for sdiv/srem does not help us, because they rely on ISD::SDIVREM / ISD::UDIVREM that we don't support either.

Expand implementation code
bool SelectionDAGLegalize::ExpandNode(SDNode *Node) {
  ...
  case ISD::UREM:
  case ISD::SREM:
    if (TLI.expandREM(Node, Tmp1, DAG))
      Results.push_back(Tmp1);
    break;
  case ISD::UDIV:
  case ISD::SDIV: {
    bool isSigned = Node->getOpcode() == ISD::SDIV;
    unsigned DivRemOpc = isSigned ? ISD::SDIVREM : ISD::UDIVREM;
    EVT VT = Node->getValueType(0);
    if (TLI.isOperationLegalOrCustom(DivRemOpc, VT)) {
      SDVTList VTs = DAG.getVTList(VT, VT);
      Tmp1 = DAG.getNode(DivRemOpc, dl, VTs, Node->getOperand(0),
                         Node->getOperand(1));
      Results.push_back(Tmp1);
    }
    break;
  }
  ...
}

bool TargetLowering::expandREM(SDNode *Node, SDValue &Result,
                               SelectionDAG &DAG) const {
  EVT VT = Node->getValueType(0);
  SDLoc dl(Node);
  bool isSigned = Node->getOpcode() == ISD::SREM;
  unsigned DivOpc = isSigned ? ISD::SDIV : ISD::UDIV;
  unsigned DivRemOpc = isSigned ? ISD::SDIVREM : ISD::UDIVREM;
  SDValue Dividend = Node->getOperand(0);
  SDValue Divisor = Node->getOperand(1);
  if (isOperationLegalOrCustom(DivRemOpc, VT)) {
    SDVTList VTs = DAG.getVTList(VT, VT);
    Result = DAG.getNode(DivRemOpc, dl, VTs, Dividend, Divisor).getValue(1);
    return true;
  }
  if (isOperationLegalOrCustom(DivOpc, VT)) {
    // X % Y -> X-X/Y*Y
    SDValue Divide = DAG.getNode(DivOpc, dl, VT, Dividend, Divisor);
    SDValue Mul = DAG.getNode(ISD::MUL, dl, VT, Divide, Divisor);
    Result = DAG.getNode(ISD::SUB, dl, VT, Dividend, Mul);
    return true;
  }
  return false;
}

As a nitpick, there is no test for error reported on srem.

@inclyc inclyc merged commit ddf85b9 into main Dec 13, 2023
3 of 4 checks passed
@inclyc inclyc deleted the users/inclyc/bpf-dont-crash-sdiv-srem branch December 13, 2023 05:41
jrguzman-ms pushed a commit to msft-mirror-aosp/platform.external.bcc that referenced this pull request Feb 1, 2024
Bug: b/308826679
Context:  llvm/llvm-project#75088
Also opened an issue upstream: iovisor/bcc#4896

Change-Id: Ia8650571dc8ba85238333f18c8e0f9b08e40f8f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants