Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various build fixes #1069

Merged
merged 3 commits into from Mar 14, 2019
Merged

Various build fixes #1069

merged 3 commits into from Mar 14, 2019

Conversation

@u1f35c
Copy link
Contributor

u1f35c commented Mar 11, 2019

This is a collection of 3 fixes I needed to get XRT building on a Debian Buster system - in particular 2 cleanups for warnings in GCC 8.3.0, plus a fixup to correctly use the local kernel-doc copy. I understand Debian is probably not a supported distro, but these fixes seem relevant to other systems.

u1f35c added 3 commits Mar 11, 2019
Compilation with GCC 8.3.0 and -Wall hits:

src/runtime_src/xocl/core/memory.h:492:18: error: argument 2 null where
non-null expected [-Werror=nonnull]
       std::memcpy(m_host_ptr,host_ptr,sz);
       ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~

host_ptr is checked earlier in the instances I can see, but doing the
additional check here silences GCC without losing the check.
GCC 8.3.0 is stricter on truncation warnings when using the str*
functions. In particular the length for strncpy() must be one less
than the total length, to allow for the trailing NULL. Fixes the
following build failures:

src/runtime_src/driver/xclng/xrt/user_gem/perf.cpp:1016:12: error:
‘char* strncpy(char*, const char*, size_t)’ specified bound 256 equals
destination size [-Werror=stringop-truncation]

     strncpy(info->device_name, device_name.c_str(), MAX_NAME_LEN);
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/xma/src/xmaapi/xmalogger.cpp:207:16: error: ‘char* strncpy(char*,
const char*, size_t)’ specified bound 40 equals destination size
[-Werror=stringop-truncation]

         strncpy(log_name, name, sizeof(log_name));
         ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The build fetches a local copy of kernel-doc if a system copy is not
found, but then stores a relative path to it which causes a build issue
when it is used. Fixes:

/bin/sh: 1: ./kernel-doc: not found
make[2]: *** [runtime_src/doc/CMakeFiles/xrt_docs.dir/build.make:90:
              runtime_src/doc/core/xmaplugin.rst] Error 127
make[2]: *** Deleting file 'runtime_src/doc/core/xmaplugin.rst'
make[1]: *** [CMakeFiles/Makefile2:315:
              runtime_src/doc/CMakeFiles/xrt_docs.dir/all] Error 2
@gbuildx

This comment has been minimized.

Copy link
Collaborator

gbuildx commented Mar 11, 2019

Can one of the admins verify this patch?

@maxzhen

This comment has been minimized.

Copy link
Collaborator

maxzhen commented Mar 11, 2019

ok to test

@gbuildx

This comment has been minimized.

Copy link
Collaborator

gbuildx commented Mar 11, 2019

Build Passed!

@@ -488,7 +488,7 @@ class buffer : public memory
// allocate sufficiently aligned memory and reassign m_host_ptr
if (posix_memalign(&m_host_ptr,alignment,sz))
throw error(CL_MEM_OBJECT_ALLOCATION_FAILURE);
if (flags & CL_MEM_COPY_HOST_PTR)
if (flags & CL_MEM_COPY_HOST_PTR && host_ptr)

This comment has been minimized.

Copy link
@maxzhen

maxzhen Mar 14, 2019

Collaborator

I'm not sure about this fix. Why check host_ptr == NULL here? It does not make sense for user to set CL_MEM_COPY_HOST_PTR and pass in NULL host_ptr, right?

This comment has been minimized.

Copy link
@u1f35c

u1f35c Mar 14, 2019

Author Contributor

It doesn't, and I think the callers do the right thing, but GCC 8.3 isn't smart enough to realise this and so bombs out with "-Werror" due to not knowing host_ptr is definitely null in this case. Adding the explicit check seemed to be the simplest way to fix the build without losing the check generally.

This comment has been minimized.

Copy link
@maxzhen

maxzhen Mar 14, 2019

Collaborator

Before the fix, if host_ptr is null, the program will crash, which is good. After the fix, we'll just silently skip copying? That does not sound right?

This comment has been minimized.

Copy link
@stsoe

stsoe Mar 14, 2019

Collaborator

A crash can never be good. The condition where host_ptr is nullptr would have been checked by APIs and have resulted in error. I agree with the fix to silence the warning.

@stsoe
stsoe approved these changes Mar 14, 2019
@maxzhen maxzhen merged commit 1261bcd into Xilinx:master Mar 14, 2019
1 check passed
1 check passed
Build pipeline Build finished.
Details
@u1f35c u1f35c deleted the u1f35c:build-fixes branch Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.