fix FORCE_READ for ez80

[coco875]

Allow to compile the output code on architecture who have register of less then 32-bit (like ez80)

[sbc100]

Why doesn't the empty asm expression work on such platforms?

[keithw]

I'm a little confused by this -- the GCC manual says that 'r' is "A register operand is allowed provided that it is in a general register.".

The size of pointer doesn't seem directly relevant here... Is the idea that there are no "general registers" capable of holding a 32-bit integer? (Why isn't this a problem for us on 32-bit architectures doing an i64.load ?)

Also, why is this relevant on an architecture that surely can't be using mprotect to do its memory enforcement...? We don't need FORCE_READ when using explicit software bounds-checks.

[coco875]

hmm maybe on clang the behavior are different ? I encounter this error with ez80-clang (who base on llvm 15, so not recent) where I end up have error of compilation when I use this macro https://godbolt.org/z/xocov6rq1

[coco875]

(to add more information about ez80, they have register who can be combine to reach 24bit but not 32 bit)

[shravanrn]

I did check and it seems that the pointer size is 3 on this ez80 test platform. However, I don't think this is the right way to fix this. From your example this looks like a specific limitation of this target/compilers for this target.

Unless we know this is a fix that applies to other platforms with similar characteristics, I think you should just add a special case for this platform

So you probably want to change the #ifdef __GNUC__ to

#if defined(__GNUC__) && !(defined(_EZ80) || defined(__ez80) || defined(__ez80__))

Above was extracted by dumping the flags from clang for this platform (Specify -dM -E flags on godbolt)

[coco875]

ok, so at the end it only check for ez80 and not architecture with less then 32 bit pointer

[shravanrn]

Fyi, the mac osx build failure is unrelated, so you can ignore that. You should fix the lint issue by running clang-format on the changes.

@sbc100 is there a way to merge a commit for now without a passing Mac build given that this failure is unrelated?

[keithw]

Sorry, I still don't understand this. Why is FORCE_READ relevant on a platform that clearly must use explicit software bounds-checks to enforce the memory bounds? That seems like the real bug we should be fixing.

FORCE_READ is only necessary to prevent the compiler from optimizing out a read when using (implicit) hardware checks.

[shravanrn]

@keithw oh yup, you're totally right. I completely missed the last line of your prior comment. This would be the right fix for the above, not the one I suggested.

Fyi, @coco875 - if you can submit a pr for the change Keith suggested that would be great. If you're not sure how to do this, let me know and I can try to get to this in the next week.

[coco875]

FORCE_READ are use in DEFINE_LOAD

#define DEFINE_LOAD(name, t1, t2, t3, force_read)                      \
  static inline t3 name##_unchecked(wasm_rt_memory_t* mem, u64 addr) { \
    t1 result;                                                         \
    wasm_rt_memcpy(&result, MEM_ADDR_MEMOP(mem, addr, sizeof(t1)),     \
                   sizeof(t1));                                        \
    force_read(result);                                                \
    return (t3)(t2)result;                                             \
  }                                                                    \
  DEF_MEM_CHECKS0(name, _, t1, return, t3)

but don't fully understand when it should be put, when WASM_RT_MEMCHECK_GUARD_PAGES are define or it's something else ?