esp-rs / rust

Rust for the xtensa architecture. Built in targets for the ESP32 and ESP8266
https://www.rust-lang.org
Other
730 stars 35 forks source link

Problems with C variadics #177

Closed bjoernQ closed 11 months ago

bjoernQ commented 1 year ago

This relates to https://github.com/esp-rs/esp-wifi/issues/16

In https://github.com/bjoernQ/xtensa-rust-variadics-problem-repo I have some very simple code to reproduce the problem.

What I do there

On RISCV it works as expected and outputs

42 "TAG" "FORMAT"
1 = 1
2 = 2
3 = 3
4 = 4
5 = 5
6 = 6
7 = 7
8 = 8
9 = 9

On ESP32 it outputs

42 "TAG" "FORMAT"
1 = 1061160316
2 = 1
3 = 2
4 = 3
5 = 2148373693
6 = 1073594208
7 = 1074266112
8 = 1073414160
9 = 4
karlri commented 1 year ago

I implemented the shortest solution I could come up with. It's not a full solution and it's not implemented in the proper place but it's provided in the case someone finds it useful.

https://github.com/esp-rs/rust/compare/esp-1.72.1.0...karlri:rust:esp-1.72.1.0

Ideally the compiler should be updated to not use the llvm intrinsic va_arg, or the va_arg intrinsic in llvm should be fixed, but I'm not competent enough to implement it where it should be implemented.

karlri commented 11 months ago

I think I traced this bug back to its source.

I believe the bug to originate from here on line 340: https://github.com/espressif/llvm-project/blob/xtensa_release_16.0.4/llvm/lib/Target/Xtensa/XtensaISelLowering.cpp

Can't Expand ISD::VAARG since we have a custom struct, we need Custom implementation. It would need to routed around line 1924

  case ISD::VAARG:
    return LowerVAARG(Op, DAG);

finally, the big job would be to implement SDValue XtensaTargetLowering::LowerVAARG(SDValue Op, SelectionDAG &DAG) const

plietar commented 11 months ago

I've opened a PR at #201 which should fix this. With it the code in the reproduction repository works as intended, and so does the esp-wifi syslog facility.

MabezDev commented 11 months ago

Closed with @plietar's patch in 1.73.0.1, thanks again @plietar! Note that not all builds are available, but by the end of the day they should be available :).