Skip to content

GH-50862: [C++][Gandiva] Fix Gandiva tests on riscv64 with an LLVM JIT relocation error - #50799

Merged
kou merged 2 commits into
apache:mainfrom
domibel:gandiva-riscv64-pic-jit
Aug 15, 2026
Merged

GH-50862: [C++][Gandiva] Fix Gandiva tests on riscv64 with an LLVM JIT relocation error#50799
kou merged 2 commits into
apache:mainfrom
domibel:gandiva-riscv64-pic-jit

Conversation

@domibel

@domibel domibel commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

On riscv64, Gandiva fails at runtime with:

JIT session error: relocation target ... is out of range of R_RISCV_HI20 fixup ...

What changes are included in this PR?

I set the PIC relocation model on the JIT TargetMachine manually to restore pre #49063 behavior. LLJIT normally sets Reloc::PIC_ for JITLink targets in prepareForConstruction, but #49063 builds the object-cache TargetMachine before that runs, so it gets the target-default (Static) model.

Are these changes tested?

Yes, by the existing Gandiva tests, on riscv64 hardware.

Are there any user-facing changes?

No public API changes. Gandiva expression evaluation now works on riscv64.

@uros-b

uros-b commented Aug 10, 2026

Copy link
Copy Markdown
Member

LGTM

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 10, 2026
@domibel

domibel commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@kou Could you please help merging this one too please?

@kou

kou commented Aug 14, 2026

Copy link
Copy Markdown
Member

Could you open an issue for this instead of using MINOR?

@domibel domibel changed the title MINOR: [C++][Gandiva] Fix Gandiva tests on riscv64 with an LLVM JIT relocation error GH-50862: [C++][Gandiva] Fix Gandiva tests on riscv64 with an LLVM JIT relocation error Aug 14, 2026
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50862 has been automatically assigned in GitHub to PR creator.

@kou kou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Comment thread cpp/src/gandiva/engine.cc Outdated
Comment on lines +143 to +145
#if defined(__riscv)
jtmb.setRelocationModel(llvm::Reloc::PIC_);
#endif

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we always enable this? (Is it safe that we enable this on non-riscv64?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The other archs are defaulting to llvm::Reloc::Static , so that change needs to be carefully tested.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's try in our CI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reloc::PIC_ was actually the default and #49063 changed it to Reloc::Static as a side effect.

https://github.com/llvm/llvm-project/blob/release/21.x/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp#L801-L839

So it looks safe to drop the riscv guard.

@github-actions github-actions Bot added awaiting review Awaiting review awaiting merge Awaiting merge awaiting committer review Awaiting committer review awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review awaiting review Awaiting review awaiting merge Awaiting merge labels Aug 14, 2026
Comment thread cpp/src/gandiva/engine.cc Outdated
@github-actions github-actions Bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 15, 2026

@kou kou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Aug 15, 2026
@kou
kou merged commit f64e90a into apache:main Aug 15, 2026
57 of 58 checks passed
@kou kou removed the awaiting merge Awaiting merge label Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants