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

thirdparty: Update kwsys #4412

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vicentebolea
Copy link
Collaborator

@vicentebolea vicentebolea commented Nov 27, 2024

Fixes: #4419

We need to update clang since no current fedora support clang9. Our sanitizer builds have many failures at the test phase but it is not reported if we cannot find any sanitizer issue. This was the case before this update, and now.

After finishing this I will move these builds to the Spack CI

@vicentebolea
Copy link
Collaborator Author

@pnorbert updating kwsys seems to have fixed this msan build. Still it passed in the minute 59 which seems very risky.

@vicentebolea vicentebolea force-pushed the update-kwsys branch 6 times, most recently from 54b12d4 to 2fc2b29 Compare December 1, 2024 00:50
@vicentebolea
Copy link
Collaborator Author

New ubsan is picking a memory align issue https://open.cdash.org/viewDynamicAnalysisFile.php?id=11240523. It is located at source/adios2/toolkit/format/bp/bp3/BP3Serializer.tcc:133.

@eisenhauer

@vicentebolea vicentebolea marked this pull request as ready for review December 1, 2024 00:52
@vicentebolea vicentebolea self-assigned this Dec 1, 2024
@eisenhauer
Copy link
Member

New ubsan is picking a memory align issue https://open.cdash.org/viewDynamicAnalysisFile.php?id=11240523. It is located at source/adios2/toolkit/format/bp/bp3/BP3Serializer.tcc:133.

That there is an issue is not surprising. The initial Span code paid zero attention to alignment concerns. @pnorbert did a fix for BP4, so maybe he can reprise it? (I'm in Australia and not returning for another week.)

@eisenhauer
Copy link
Member

eisenhauer commented Dec 1, 2024

That there is an issue is not surprising. The initial Span code paid zero attention to alignment concerns. @pnorbert did a fix for BP4, so maybe he can reprise it? (I'm in Australia and not returning for another week.)

Nevermind, this is a different issue. I assumed that alignment associated with spans was related to the data pointer returned to applications. That is not the issue here. The issue seems to be that the min/max aren't aligned in the metadata buffer. But they don't really have to be. What we should be done differently is this (line 131 in BP3Serializer::PutSpanMetadata):

        std::copy(&min, &min + 1, reinterpret_cast<T *>(buffer.data() + minPosition));
        std::copy(&max, &max + 1, reinterpret_cast<T *>(buffer.data() + maxPosition));

This is moving min/max values to an address that is potentially misaligned for their datatype, and so we shouldn't be using std::copy. I'd change this to something like:

std::memcpy(buffer.data()+minPosition, &min, sizeof(T));
std::memcpy(buffer.data()+maxPosition, &max, sizeof(T));

There may be some casts necessary to get that to compile cleanly, but I'll let y'all sort that, I've got koalas to pet...

@vicentebolea vicentebolea force-pushed the update-kwsys branch 2 times, most recently from 033e1bd to b3de92e Compare December 6, 2024 22:11
kwrobot and others added 2 commits December 6, 2024 17:14
Code extracted from:

    https://gitlab.kitware.com/utils/kwsys.git

at commit dfbcd769aea25157ee65a92f941d0e233fe79826 (master).

Upstream Shortlog
-----------------

Ben Boeckel (1):
      72e677e9 kwsysPrivate.h: Remove unused build-tree copy

Brad King (11):
      9f9ff427 SystemTools: Teach RemoveADirectory to handle non-readable directories
      3c922475 Convert http URLs to https
      beaf1ca1 ConsoleBuf: Fix test case when running under Windows Terminal
      4feb470a SystemTools: Remove GetActualCaseForPath from CollapseFullPath
      6e847d08 SystemInformation: Add missing EOF check when reading /proc/cpuinfo
      741c9c96 SystemTools: Expose GetActualCaseForPathCached publicly
      fdf4f2f8 SystemTools: Fix ReadSymlink for links to absolute paths on Windows
      20b2c992 SystemTools: Remove unused global object
      47dce1a3 SystemTools: Remove path translation map
      30e9db2d SystemTools: Drop GetActualCaseForPathCached
      5995fd7d SystemInformation: Ignore stderr from OS query tools

Christoph Grüninger (3):
      d3f3c38b Use prefix ++ operators for non-primitive types
      9fd52415 Range-for loop with const reference
      2310be95 Make variable more local

Clemens Wasser (1):
      dd7d92d6 SystemTools: Implement GetEnv via GetEnvironmentVariableW on Win32

Daniel Pfeifer (1):
      68e1f35d SystemInformation: fix use of using

Jochem van Boxtel (1):
      510b13b4 SystemTools: Add FileId class and GetFileId function

Juan Ramos (2):
      ff14b4f5 SystemInformation: Fix find logic
      a347a66b SystemInformation: Implement HasFPU on Apple processors

Mike Lundy (1):
      ee3223d7 SystemTools: fix clonefile optimization on macOS

Peter Kokot (1):
      967b2120 Remove legacy unused IMMEDIATE CMake keyword

scivision (11):
      12825be6 lint: use foreach(... IN {ITEMS,LISTS} ...)
      f10cb6ad lint: use modern add_test(NAME ... COMMAND ...)
      ebb95153 lint: set_property(TEST
      f26b1b39 SystemInformation: use std::cerr like rest of KWSys
      3c403fa9 SystemInformation: Replace C-style cast with reinterpret_cast
      7f4459d5 Comeau: Remove undocumented support for this compiler
      6624edf2 SystemTools:FileIs{Directory,Executable,FIFO}: refactor for simplicity
      5f4012c6 Don't compare to nullptr
      3f14fce6 Process: Replace 0 with NULL in pointer arguments to GetFullPathNameW
      210cea0a SystemTools: Replace unused argument to GetFullPathNameW with nullptr
      f5e82d63 SystemTools: Replace malloc() with std::string
# By KWSys Upstream
* upstream-KWSys:
  KWSys 2024-11-30 (dfbcd769)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Sanitizer builds
3 participants