[Lldb-commits] [PATCH] D82064: [ARM64] Add QEMU testing environment setup guide for SVE testing

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like this. Apart from the inline comments (which are mostly about removing excessive mentions of arm(64) -- I want to make that text feel generic, since the file is in a generic place), I have one high-level comment. Given the current layout of the repo, it seems like these scripts would be better placed under `lldb/scripts` than `lldb/tools`. E.g., this scripts are fairly similar to the `macos-setup-codesign.sh` script, which also lives in that folder.



================
Comment at: lldb/tools/lldb-test-qemu/README.txt:2
+----------------------------------------------------------------------
+  LLDB testing on Arm/AArch64 Linux using QEMU system mode emulation
+----------------------------------------------------------------------
----------------



================
Comment at: lldb/tools/lldb-test-qemu/README.txt:9-14
+Main motivation for writing this guide is to test AArch64 features like SVE,
+MTE, Pointer Authentication etc. These scripts can be used as reference to set
+up similar testing environment for other architectures supported by QEMU.
+
+We have written helper scripts under llvm-project/lldb/tools/lldb-test-qemu
+which can help quickly setup a Arm or AArch64 testing environment using QEMU.
----------------



================
Comment at: lldb/tools/lldb-test-qemu/README.txt:16
+
+* setup.sh is used to build AArch64 and Arm Linux kernel image and QEMU
+  system emulation executable(s) from source.
----------------



================
Comment at: lldb/tools/lldb-test-qemu/README.txt:20
+* rootfs.sh is used to generate Ubuntu root file system images to be used for
+  QEMU Arm and AArch64 system emulation.
+
----------------



================
Comment at: lldb/tools/lldb-test-qemu/README.txt:22
+
+* run-qemu.sh utilizes QEMU to boot an Arm or AArch64 Linux kernel image with
+  a given root file system image.
----------------



================
Comment at: lldb/tools/lldb-test-qemu/rootfs.sh:57-65
+if [[ "$rfs_arch" != "arm64" && "$rfs_arch" != "armhf" ]]; then
+  echo "Invalid architecture: $rfs_arch"
+  print_usage 1
+fi
+
+if [[ "$rfs_distro" != "focal" && "$rfs_distro" != "bionic" ]]; then
+  echo "Invalid distribution: $rfs_distro"
----------------
You're only passing these to `qemu-debootstrap`. It sounds like things could "just work" for a bunch of other distro/arch combinations if we just removed these checks.


================
Comment at: lldb/tools/lldb-test-qemu/run-qemu.sh:83-87
+  if [[ "$ARCH" == "arm" ]]; then
+    QEMU_BIN=$(pwd)/qemu.git/arm-softmmu/qemu-system-arm
+  elif [[ "$ARCH" == "arm64" ]]; then
+    QEMU_BIN=$(pwd)/qemu.git/aarch64-softmmu/qemu-system-aarch64
+  fi
----------------
It would be nice if this searched for the qemu binary on the PATH if it was not explicitly provided. Something like:
```
if [ -d qemu.git ]; then
  QEMU_BIN=$(pwd)/qemu.git/arm-softmmu/qemu-system-arm
else
  QEMU_BIN=qemu-system-arm
fi
```


================
Comment at: lldb/tools/lldb-test-qemu/run-qemu.sh:99-101
+  if [[ $SVE ]]; then
+    invalid_arg "--sve"
+  fi
----------------
How about we check for this in the parsing while loop? That would mean the --arch argument has to come before --sve, but that does not seem like a bad thing


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

https://reviews.llvm.org/D82064