[libc-commits] [PATCH] D145584: [libc] Add support for setjmp and longjmp in riscv

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Mostly LGTM but I have left some questions inline. About the structuring, to avoid complicated conditionals in CMake and source code both, we should structure it this way:

  src/setjmp/
     - CMakeLists.txt
     - setjmp.h
     - longjmp.h
     - riscv64/
          - CMakeLists.txt
          - setjmp.cpp
          - longjmp.cpp
     - x86_64/
         - CMakeLists.txt
         - setjmp.cpp
         - longjmp.cpp

In `riscv64/CMakeLists.txt`:

  add_entrypoint_object(
    setjmp
    SRCS
      setjmp.cpp
    HDRS
      ../setjmp.h
    COMPILE_OPTIONS
      ... # RISCV specific compile options
  )
  
  add_entrypoint_object(
    longjmp
    SRCS
      longjmp.cpp
    HDRS
      ../longjmp.h
    COMPILE_OPTIONS
      ... # RISCV specific compile options
  )

Similar `CMakeLists.txt` file should be added in the `x86_64` directory also. Next, we should add `ALIAS` targets in `src/setjmp/CMakeLists.txt`:

  if(NOT EXISTS )
    return 
  endif()
  
  add_entrypoint_object(
    setjmp
    ALIAS
    DEPENDS
      .${LIBC_TARGET_ARCH}.setjmp
  )
  
  add_entrypoint_object(
    longjmp
    ALIAS
    DEPENDS
      .${LIBC_TARGET_ARCH}.longjmp
  )



================
Comment at: libc/src/setjmp/riscv64/longjmp.h:22
+LLVM_LIBC_FUNCTION(void, longjmp, (__jmp_buf * buf, int val)) {
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wuninitialized"
----------------
You shouldn't need this for the riscv flavor.


================
Comment at: libc/src/setjmp/riscv64/longjmp.h:24
+#pragma GCC diagnostic ignored "-Wuninitialized"
+  LIBC_INLINE_ASM("ld ra, %0\n\t" : "=m"(buf->__pc));
+  LIBC_INLINE_ASM("ld s0, %0\n\t" : "=m"(buf->__regs[0]));
----------------
Should this be `"=m"` or just `"m"`?


================
Comment at: libc/src/setjmp/riscv64/longjmp.h:56
+
+  LIBC_INLINE_ASM("seqz %0, %1" : "+r"(buf) : "r"(val) :);
+  LIBC_INLINE_ASM("add %0, %0, %1" : "+r"(buf) : "r"(val), "r"(buf) :);
----------------
Why is this required? Can we just copy `val` to `a0` and return? Or, even better would be just `return val;` if we can make this a `naked` function?


================
Comment at: libc/src/setjmp/riscv64/setjmp.h:57-58
+
+  LIBC_INLINE_ASM("li %0, 0" : "+r"(buf) : :);
+  LIBC_INLINE_ASM("ret" : : :);
+}
----------------
Can this be replaced with `return 0;`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145584/new/

https://reviews.llvm.org/D145584