Skip to content

StRoot/StTriggerUtilities: replace sys/types.h with stdint.h#212

Merged
veprbl merged 1 commit intostar-bnl:mainfrom
veprbl:pr/stdint_StRoot_StTriggerUtilities
Dec 3, 2021
Merged

StRoot/StTriggerUtilities: replace sys/types.h with stdint.h#212
veprbl merged 1 commit intostar-bnl:mainfrom
veprbl:pr/stdint_StRoot_StTriggerUtilities

Conversation

@veprbl
Copy link
Copy Markdown
Member

@veprbl veprbl commented Nov 23, 2021

ROOT5/cint doesn't always work with sys/types.h, types from stdint are the ISO C standard

@plexoos
Copy link
Copy Markdown
Member

plexoos commented Nov 23, 2021

doesn't always work

Do we understand the conditions under which it does work? There is clearly a difference between the container and RACF environments since we don't see the problem there.

@veprbl
Copy link
Copy Markdown
Member Author

veprbl commented Nov 23, 2021

doesn't always work

Do we understand the conditions under which it does work? There is clearly a difference between the container and RACF environments since we don't see the problem there.

I haven't looked, but my guess is that rootcint could pick up glibc headers installed from spack.

@veprbl
Copy link
Copy Markdown
Member Author

veprbl commented Nov 23, 2021

Also, it's not like we never see the issue on RACF. Here is an error from Gene's email in the "StDbLib regression" email thread:

Error: class,struct,union or type {int__val[2];} not defined  /usr/include/bits/typesizes.h:73:
Error: Symbol __extension__typedef__DEV_T_TYPE__dev_t is not defined in current scope  /usr/include/bits/types.h:133:
Error: Symbol __extension__typedef__UID_T_TYPE__uid_t is not defined in current scope  /usr/include/bits/types.h:134:
Error: Symbol __extension__typedef__GID_T_TYPE__gid_t is not defined in current scope  /usr/include/bits/types.h:135:
Error: Symbol __extension__typedef__INO_T_TYPE__ino_t is not defined in current scope  /usr/include/bits/types.h:136:
Error: Symbol __extension__typedef__INO64_T_TYPE__ino64_t is not defined in current scope  /usr/include/bits/types.h:137:
Error: Symbol __extension__typedef__MODE_T_TYPE__mode_t is not defined in current scope  /usr/include/bits/types.h:138:
Error: Symbol __extension__typedef__NLINK_T_TYPE__nlink_t is not defined in current scope  /usr/include/bits/types.h:139:
Error: Symbol __extension__typedef__OFF_T_TYPE__off_t is not defined in current scope  /usr/include/bits/types.h:140:
Error: Symbol __extension__typedef__OFF64_T_TYPE__off64_t is not defined in current scope  /usr/include/bits/types.h:141:
Error: Symbol __extension__typedef__PID_T_TYPE__pid_t is not defined in current scope  /usr/include/bits/types.h:142:
Error: Symbol __extension__typedef__FSID_T_TYPE__fsid_t is not defined in current scope  /usr/include/bits/types.h:143:
Error: Symbol __extension__typedef__CLOCK_T_TYPE__clock_t is not defined in current scope  /usr/include/bits/types.h:144:
Error: Symbol __extension__typedef__RLIM_T_TYPE__rlim_t is not defined in current scope  /usr/include/bits/types.h:145:
Error: Symbol __extension__typedef__RLIM64_T_TYPE__rlim64_t is not defined in current scope  /usr/include/bits/types.h:146:
Error: Symbol __extension__typedef__ID_T_TYPE__id_t is not defined in current scope  /usr/include/bits/types.h:147:
Error: Symbol __extension__typedef__TIME_T_TYPE__time_t is not defined in current scope  /usr/include/bits/types.h:148:
Error: Symbol __extension__typedef__USECONDS_T_TYPE__useconds_t is not defined in current scope  /usr/include/bits/types.h:149:
Error: Symbol __extension__typedef__SUSECONDS_T_TYPE__suseconds_t is not defined in current scope  /usr/include/bits/types.h:150:
Error: Symbol __extension__typedef__DADDR_T_TYPE__daddr_t is not defined in current scope  /usr/include/bits/types.h:152:
Error: Symbol __extension__typedef__KEY_T_TYPE__key_t is not defined in current scope  /usr/include/bits/types.h:153:
Error: Symbol __extension__typedef__CLOCKID_T_TYPE__clockid_t is not defined in current scope  /usr/include/bits/types.h:156:
Error: Symbol __extension__typedef__TIMER_T_TYPE__timer_t is not defined in current scope  /usr/include/bits/types.h:159:
Error: Symbol __extension__typedef__BLKSIZE_T_TYPE__blksize_t is not defined in current scope  /usr/include/bits/types.h:162:
Error: Symbol __extension__typedef__BLKCNT_T_TYPE__blkcnt_t is not defined in current scope  /usr/include/bits/types.h:167:
Error: Symbol __extension__typedef__BLKCNT64_T_TYPE__blkcnt64_t is not defined in current scope  /usr/include/bits/types.h:168:
Error: Symbol __extension__typedef__FSBLKCNT_T_TYPE__fsblkcnt_t is not defined in current scope  /usr/include/bits/types.h:171:
Error: Symbol __extension__typedef__FSBLKCNT64_T_TYPE__fsblkcnt64_t is not defined in current scope  /usr/include/bits/types.h:172:
Error: Symbol __extension__typedef__FSFILCNT_T_TYPE__fsfilcnt_t is not defined in current scope  /usr/include/bits/types.h:175:
Error: Symbol __extension__typedef__FSFILCNT64_T_TYPE__fsfilcnt64_t is not defined in current scope  /usr/include/bits/types.h:176:
Error: Symbol __extension__typedef__FSWORD_T_TYPE__fsword_t is not defined in current scope  /usr/include/bits/types.h:179:
Error: Symbol __extension__typedef__SSIZE_T_TYPE__ssize_t is not defined in current scope  /usr/include/bits/types.h:181:
Error: Symbol __extension__typedef__SYSCALL_SLONG_TYPE__syscall_slong_t is not defined in current scope  /usr/include/bits/types.h:184:
Error: Symbol __extension__typedef__SYSCALL_ULONG_TYPE__syscall_ulong_t is not defined in current scope  /usr/include/bits/types.h:186:
Error: class,struct,union or type __off64_t not defined  /usr/include/bits/types.h:190:
Error: Symbol __extension__typedef__SWORD_TYPE__intptr_t is not defined in current scope  /usr/include/bits/types.h:195:
Error: Symbol __extension__typedef__U32_TYPE__socklen_t is not defined in current scope  /usr/include/bits/types.h:198:
Warning: Error occurred during reading source files
Warning: Error occurred during dictionary source generation
!!!Removing .sl73_gcc485/obj/StRoot/StSpinPool/StFmsTriggerMaker/StFmsTriggerMaker_Cint.cxx .sl73_gcc485/obj/StRoot/StSpinPool/StFmsTriggerMaker/StFmsTriggerMaker_Cint.h !!!
Error: rootcint: error loading headers...
cons: *** [.sl73_gcc485/obj/StRoot/StSpinPool/StFmsTriggerMaker/StFmsTriggerMaker_Cint.cxx] Error 2
cons: errors constructing .sl73_gcc485/obj/StRoot/StSpinPool/StFmsTriggerMaker/StFmsTriggerMaker_Cint.h

@veprbl veprbl mentioned this pull request Nov 24, 2021
11 tasks
@starsdong
Copy link
Copy Markdown
Member

Hongwei, it seems that we need your review/approval, particularly on the changes to the StHltMaker. Thanks

@kehw
Copy link
Copy Markdown
Member

kehw commented Dec 1, 2021

Hongwei, it seems that we need your review/approval, particularly on the changes to the StHltMaker. Thanks

Hi @starsdong , I have made a comment and waiting for reply. I have no problem to the code changes. However, I am a bit confused about the intention. If it is urgently needed, I have no problem to approve it.

Copy link
Copy Markdown
Member

@zlchang zlchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also approved the request. It might be better to add a new header file, using typedef to define for example uchar as unsigned char, and just remove the <sys/types.h>. It makes much less edits to the original files.

@veprbl
Copy link
Copy Markdown
Member Author

veprbl commented Dec 1, 2021

@kehw I don't see any comments by you in this PR.

I also approved the request. It might be better to add a new header file, using typedef to define for example uchar as unsigned char, and just remove the <sys/types.h>. It makes much less edits to the original files.

There is no point in keeping several type naming conventions. The edits are quite straightforward and the substitutions themselves were performed using a script.

@kehw
Copy link
Copy Markdown
Member

kehw commented Dec 1, 2021

@veprbl if you scroll up this page, you can see my questions. I agree that the code changes are quite straightforward and I have no problem on that. I am just a bit confused about what problem you are trying to solve. Did someone try to build it on macOS? I assume that is what is the macro __APPLE__ for.

**************************************************************/
#ifdef __APPLE__
#include <sys/types.h>
#endif
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header sys/types.h was removed at two locations. Both defined for APPLE. I do not see stdint.h included at any place and all other changes are deal with self-introduced typedef for unsigned short? May I know what problem is this PR trying to fix?

Copy link
Copy Markdown
Member Author

@veprbl veprbl Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review. The reason why the include is "not needed" on non-apple here is because the stdlib.h shipped with glibc happens to #include sys/types.h under the hood (by an accidental reason). This behavior is glibc-specific and likely will not work with another libc. The sys/types.h include should have been unconditional here, this works only by accident.

The reason why including stdlib.h will work with cint is because it provides its own stub for it (see cint/cint/include/stdlib.h) that doesn't include sys/types.h.

I am removing use of non-standard types coming from sys/types.h from entire codebase to ensure that even if people have not missed the failing sys/types.h include, we would still be able to remove it with confidence.

The practical problem here is that CINT is not able to work with modern libc's, as it's not compliant and outdated. There are alternative approaches we could take here, but that would require recompiling the ROOT5, or switching to ROOT6 entirely.

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.

6 participants