Fixed coverity issue in acl_kernel_if.cpp

[haoxian2]

Continuation of discussion featured in #220.
Fixed coverity issue in acl_kernel_if.cpp: Type: Reliance on integer endianness (INCOMPATIBLE_CAST)

In the three instances where this issue happens, the function acl_kernel_if_read_32b is used at the end of the scope to query about what's inside the acl_kernel_if* kern object. However, they don't do anything with the obtained information. I believe this read operation is performed to confirm that the acl_kernel_if* kern has been written properly. So if the read operation successed, then the write operation must have also succeeded as well. The functions employing acl_kernel_if_read_32b are used to return the segment offset. This segment offset is then used to perform another read/write, which return status would then be used for error checking. Therefore, the acl_kernel_if_read_32b function is redundant and can be removed from the aformentioned 3 instances.

[haoxian2]

The function acl_kernel_if_read_32b does r = kern->io.read(&kern->io, (dev_addr_t)addr, (char *)val, (size_t)size) as a read of val and if r < size then it does a print and return -1 and kern->io.printf( "HAL Kern Error: Read failed from addr %x, read %zu expected %zu\n", addr, r, size). So technically it doesn't really do anything except printing out information if the read somehow failed. Therefore, the val doesn't need to be initialized.

Investigating the intent behind the discarded read in more detail.

[haoxian2]

After some investigation, I found the following:

• The function in question in r = kern->io.read(&kern->io, (dev_addr_t)addr, (char *)val, (size_t)size) is kern->io.read which is found in acl_bsp_io.h. It is found in the form of size_t (*read)(acl_bsp_io *io, dev_addr_t src, char *dest, size_t size)
• It's a function pointer, but I can't seem to find the definition of where it is pointing to.
  @pcolberg @zibaiwan Do you know where this pointer is pointing to?

[zibaiwan]

After some investigation, I found the following:

• The function in question in r = kern->io.read(&kern->io, (dev_addr_t)addr, (char *)val, (size_t)size) is kern->io.read which is found in acl_bsp_io.h. It is found in the form of size_t (*read)(acl_bsp_io *io, dev_addr_t src, char *dest, size_t size)
• It's a function pointer, but I can't seem to find the definition of where it is pointing to.
  @pcolberg @zibaiwan Do you know where this pointer is pointing to?

Hi @haoxian2 , I think it's acl_kernel_if_read.

bsp_io_kern[physical_device_id].read = acl_kernel_if_read;

static size_t acl_kernel_if_read(acl_bsp_io *io, dev_addr_t src, char *dest,
                                 size_t size) {
  acl_assert_locked_or_sig();

  ACL_HAL_DEBUG_MSG_VERBOSE(5,
                            "HAL Reading from Kernel: %zu bytes %zx -> %zx\n",
                            size, (size_t)src, (size_t)dest);
  return io->device_info->mmd_dispatch->aocl_mmd_read(
             io->device_info->handle, NULL, size, (void *)dest,
             kernel_interface, (size_t)src) == 0
             ? size
             : 0;
}

Kindly let me know if you have more questions of this function.

[haoxian2]

After a bit of digging, I concluded that acl_kernel_if_read_32b only does a read by only modifying the segment variable. It is employed to confirmed that the previous write operation executed successfully. However, this is redundant since the function that employs acl_kernel_if_read_32b are used to return a segment_offset which is then used in a read/write operation again, but this time, there is an error checking in place for if the operation was successful. For example, in acl_kernel_cra_set_segment, acl_kernel_if_read_32b is used for a discarded read at the end of the scope to the segment variable. One instance of acl_kernel_cra_set_segment usage is in acl_kernel_cra_read, where after uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr), it returns if a read at the segment_offset was successful or not.

Since the subsequent read/write already checks whether or not the previous write operation is successful, we don't need the discarded read at the end of the scope.

[zibaiwan]

Thanks Hao for fixing this. I need a bit more time to review this.

[zibaiwan]

@haoxian2 , I reviewed the very first swarm change that introduced the following two lines. It mainly intended to add the kern->cur_segment = segment;, the rest of two occurrences were just copied from this code.

 kern->cur_segment = segment;
 acl_kernel_if_read_32b(kern, OFFSET_KERNEL_CRA_SEGMENT,
                           (unsigned int *)&segment);

I agree with you analysis that the acl_kernel_if_read_32b can be deleted. The read is going into the local segment variable which is not propagated to anywhere.

In case we miss anything, can you please rebase your branch to the latest runtime codebase and open a PR against acl repo to do the integration testing?

Thanks,

Zibai

[zibaiwan]

@haoxian2 , thanks!