Fix macro generation

[kr-sc]

1. We need to add ULL modifier to all values to fix some issues with set_field and get_field OpenOCD macros (for example, when mask = 2 << 31, it leads to UB)

[timsifive]

Does this still let OpenOCD build using mingw32? ISTR that was the reason to add the extension.

[kr-sc]

Does this still let OpenOCD build using mingw32? ISTR that was the reason to add the extension.

Yes, I successfully build OpenOCD with mingw32.

[kr-sc]

ISTR that was the reason to add the extension.

Just a double-check: what is extension are you talking about? And what does ISTR mean?

[kr-sc]

Upd: I have a problems with testing OpenOCD on riscv-tests with new debug_defines.h/c (ETrigger, ICount and ITrigger fail). I'm investigating.

[timsifive]

Just a double-check: what is extension are you talking about? And what does ISTR mean?

By "extension" I meant "ULL". ISTR=I Seem To Recall

[kr-sc]

Previous version of this patch leads to failure on some (look above) riscv-tests. After I replace U with ULL, it was fixed.

Short reproduction of occured problem you can see here: https://godbolt.org/z/z6n7xd71s (change MASK from 0x3fULL to 0x3fU to see difference). When we try to set 1 with mask 0x3fU to 0x5800000000000000 , hight 32 bits of value become zero. It happened because ~(0x3fU) is a 0xffffffc0, so after conversion to uint64_t we lost hight 32 bits.

[timsifive]

Looks good overall.

[timsifive]

Suggested change

	sufffix = "ULL" if unsigned else ""
	suffix = "ULL" if unsigned else ""

[kr-sc]

Addressed

[timsifive]

I just tried to use the output of this with OpenOCD, and it fails to build because of a bunch of printf() using %d for what are now ULL values. How did you resolve that?

Looking at your godbolt example again, doesn't it make more sense to change the shift macro?

[kr-sc]

I just tried to use the output of this with OpenOCD, and it fails to build because of a bunch of printf() using %d for what are now ULL values. How did you resolve that?

I just replace %d with correct specifier in all such places.

Looking at your godbolt example again, doesn't it make more sense to change the shift macro?

We thoughr about it at internal discussion (with @aap-sc and @en-sc ) and came to conclusion that we need both changes: fix macro and update suffixes. I work on patch for shift macro now.

[timsifive]

I just replace %d with correct specifier in all such places.

That seems like a lot of unnecessary churn. Here's an example of an OpenOCD build error if we use the latest version:

../src/helper/log.h:150:19: error: format '%lx' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
  150 |         LOG_ERROR("[%s] " fmt_str, target_name(target), ##__VA_ARGS__)
      |                   ^~~~~~~
../src/helper/log.h:124:68: note: in definition of macro 'LOG_ERROR'
  124 |         log_printf_lf(LOG_LVL_ERROR, __FILE__, __LINE__, __func__, expr)
      |                                                                    ^~~~
../src/target/riscv/riscv-013.c:4952:9: note: in expansion of macro 'LOG_TARGET_ERROR'
 4952 |         LOG_TARGET_ERROR(target, "Unknown DCSR cause field: 0x%" PRIx64, get_field(dcsr, CSR_DCSR_CAUSE));
      |         ^~~~~~~~~~~~~~~~

Because the new definition of CSR_DCSR_CAUSE is:

#define CSR_DCSR_CAUSE                      0x1c0ULL

There is no reason for that constant to have any suffix at all. The original code only applied the suffix for large constants, which seems cleaner.

What specific bug did you originally encounter? Which fields were involved?

[kr-sc]

What specific bug did you originally encounter? Which fields were involved?

I catch UB on OpenOCD with Sanitizer on riscv-013.c:5064

dmcontrol = set_field(dmcontrol, DM_DMCONTROL_RESUMEREQ, 0);

[kr-sc]

There is no reason for that constant to have any suffix at all. The original code only applied the suffix for large constants, which seems cleaner.

As I write above, we decided to apply U suffixes to all such constants. At first version of patch I apply U or ULL depends on value, but get an error in riscv-test (comment above). So, after that we decided to make all such constants ULL.

[en-sc]

The original issue was with undefined behavior due to integer overflow in operations like so:

dmcontrol = set_field(dmcontrol, DM_DMCONTROL_RESUMEREQ, 0);

set_field macro calculates DM_DMCONTROL_RESUMEREQ << 1 which does not fit an int.
(riscv013_step_or_resume_current_hart())

To mitigate this, such constants were made unsigned.

As for LL part:

#define set_field(reg, mask, val) (((reg) & ~(mask)) | (((val) * ((mask) & ~((mask) << 1))) & (mask)))

If val is an int and mask is unsigned int, (val) * ((mask) & ~((mask) << 1) can be 0 (unsigned overflow), so the result will be zero (The comment with an example).

An alternative approach is to use a direct cast to a fixed-length integer types for masks (depending on the size of the register):

#define CSR_DCSR_CAUSE                      (uint32_t)0x1c0

[timsifive]

There are 3 types of values in debug_defines.h:

1. Offsets, which cannot be negative so unsigned makes sense.
2. Lengths, which also cannot be negative. Same.
3. Masks, which also can't be negative. Same.

So I agree that every number should be unsigned. But masks of 32-bit registers (such as dmcontrol) shouldn't be 64-bit. Field offsets and lengths shouldn't be 64-bit either. It just feels off.

What if we use this function instead of the macro? Seems to work. Less messy than macros in any case, and the types are very explicit.

static inline uint64_t set_field(uint64_t reg, uint64_t mask, uint64_t val)
{
	/* Clear current value from field. */
	reg &= ~mask;
	uint64_t low_field_bit = mask & ~(mask << 1);
	reg |= (val * low_field_bit) & mask;
	return reg;
}