This is an archive of the discontinued LLVM Phabricator instance.

Recognize all NetBSD architectures in UBSan
ClosedPublic

Authored by krytarowski on Feb 7 2018, 7:32 AM.

Details

Summary

Use uniform accessors for Program Pointer,
Stack Pointer and Frame Pointer.

Remove CPU check in UBSan supported platforms
and rely only on the OS type.

This adds NetBSD support in GetPcSpBp() for:

  • ARM
  • ARM64
  • HPPA
  • PowerPC/PowerPC64
  • SPARC/SPARC64
  • MIPS
  • DEC Alpha AXP
  • DEC VAX
  • M68K and M68010
  • SH3
  • IA64
  • OR1K
  • RISCV

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Feb 7 2018, 7:32 AM
ro added a comment.Feb 7 2018, 8:18 AM

I'm not sure I like it this way: the patch seems extremely repetive to me. Couldn't this just (with the exception of sparc/sparcv9) define a common macro
for the bp register number and then do

          *pc = _UC_MACHINE_PC(ucontext);
	​  *sp = _UC_MACHINE_SP(ucontext);
	​  *bp = ucontext->uc_mcontext.__gregs[_REG_BP];

Maybe even factor out

ucontext_t *ucontext = (ucontext_t*)context;

in all cases?

Another nit (while you are fixing inconsistencies): we have both `(ucontext_t*)` and `(ucontext_t *)` (i.e. with and without a space) in casts.

There is an option to to attempt to push _UC_MACHINE_BP to the sources of NetBSD for all ports.

So all NetBSD ports will use _UC_MACHINE_().

What do you think?

ro added a comment.Feb 7 2018, 8:30 AM

That's even better, of course. Way more readable.

How about the CAN_SANITIZER_UB part?

ro added a comment.Feb 7 2018, 8:41 AM

TBH, I've always disliked it that way: my preference would be to go for the SANITIZE_<PLATFORM> forms as far as possible.

I've proposed this patch in the NetBSD source code:

http://netbsd.org/~kamil/patch-00044-_UC_MACHINE_BP.txt

Thread: http://mail-index.netbsd.org/tech-userlevel/2018/02/07/msg011106.html

The UBSan ifdef could be changed to check only the kernel and skipping CPU.

  • use _UC_MACHINE_FP() for all NetBSD architectures
  • cover all NetBSD architectures with the same code in GetPcSpBp()
  • remove check for CPU types in lib/ubsan/ubsan_platform.h
krytarowski retitled this revision from Recognize a buch of CPU/NetBSD ports of GetPcSpBp() to Recognize all NetBSD architectures in UBSan.Feb 15 2018, 6:02 PM
krytarowski edited the summary of this revision. (Show Details)
vitalybuka added inline comments.Feb 16 2018, 11:18 AM
lib/ubsan/ubsan_platform.h
16

Why did you remove CPU check?
How can we be sure that e.g. Linux on any CPU is going to work?

krytarowski added inline comments.Feb 16 2018, 11:26 AM
lib/ubsan/ubsan_platform.h
16

We already maintain it with CMake rules.

As I understand this was proposed by @ro to just maintain the list of OSes here. I prefer this over the existing situation.

If we want to control CPU types here, I would prefer to split this per OS and check CPU. LINUX || FREEBSD || APPLE || NETBSD working on the same set of CPUs is wrong.

vitalybuka accepted this revision.Feb 16 2018, 4:55 PM
This revision is now accepted and ready to land.Feb 16 2018, 4:55 PM
This revision was automatically updated to reflect the committed changes.