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

BUILD: drmemory build failure caused by strcasestr declaration error #2522

Open
xdje42 opened this issue Oct 4, 2024 · 7 comments · May be fixed by #2526
Open

BUILD: drmemory build failure caused by strcasestr declaration error #2522

xdje42 opened this issue Oct 4, 2024 · 7 comments · May be fixed by #2526
Assignees

Comments

@xdje42
Copy link
Contributor

xdje42 commented Oct 4, 2024

Describe the bug

strcasestr's definition is char *(const char *, const char *) whereas drmemory/common/utils.h uses the older
const char *(const char *, const char *).
This causes a build failure on linux (64-bit, recent vintage).

In file included from /tmp/drmemory/common/utils_shared.c:30:
/usr/include/string.h:380:14: error: conflicting types for ‘strcasestr’; have ‘char *(const char *, const char *)’
  380 | extern char *strcasestr (const char *__haystack, const char *__needle)
      |              ^~~~~~~~~~
In file included from /tmp/drmemory/common/utils_shared.c:28:
/tmp/drmemory/common/utils.h:1015:1: note: previous declaration of ‘strcasestr’ with type ‘\
const char *(const char *, const char *)’
 1015 | strcasestr(const char *text, const char *pattern);
      | ^~~~~~~~~~

To Reproduce

Steps to reproduce the behavior:

$ git clone https://github.com/DynamoRIO/drmemory.git
$ cd drmemory
$ ./make/git/dev-setup.sh
$ cd ..
$ mkdir build
$ cd build
$ cmake ../drmemory
$ make -k # other failures may be present: -k keeps going so that strcasestr failure is seen

Versions

  • What version of Dr. Memory are you using? git head @bfbc4a118a58a182770d306120cad41b9893c53
@derekbruening
Copy link
Contributor

strcasestr's definition is char *(const char *, const char *) whereas drmemory/common/utils.h uses the older
const char *(const char *, const char *).
This causes a build failure on linux (64-bit, recent vintage).

How much older? If this is changed is it going to break older toolchains?

@xdje42
Copy link
Contributor Author

xdje42 commented Oct 4, 2024

Re: older toolchains: I'm not sure, but I suspect it doesn't matter: Note that the fix would presumably be to just copy the check used by drltrace already present in the tree.

@xdje42
Copy link
Contributor Author

xdje42 commented Oct 4, 2024

Actually, I was being optimistic. The hits are all over the tree and applying a surgical fix is a lot of work.

I went back through older glibc versions. strcasestr was added here (heh, back when I worked at Cygnus :-) ):

1997-11-26 04:28  Ulrich Drepper  <[email protected]>
[...]
        * string/Makefile (routines): Add strcasestr.
        * string/string.h: Add prototype for strcasestr.
        * sysdeps/generic/strcasestr.c: New file.

I downloaded glibc-2.1.1 from 1999 and it has char* for the return type, not drmemory's const char*.
If we're not ok to just fix drmemory's copy, at least #ifdef LINUX, then that would be disappointing but alas not unexpected.

The original definition (from glibc 2.1.1) was:

#ifdef __USE_GNU
/* Similar to `strstr' but this function ignores the case of both strings.  */
extern char *__strcasestr __P ((__const char *__haystack,
                                __const char *__needle));
extern char *strcasestr __P ((__const char *__haystack,
                              __const char *__needle));
#endif

Today's definition (from glibc 2.40) is:

#ifdef __USE_MISC
/* Similar to `strstr' but this function ignores the case of both strings.  */
# ifdef __CORRECT_ISO_CPP_STRING_H_PROTO
extern "C++" char *strcasestr (char *__haystack, const char *__needle)
     __THROW __asm ("strcasestr") __attribute_pure__ __nonnull ((1, 2));
extern "C++" const char *strcasestr (const char *__haystack,
                                     const char *__needle)
     __THROW __asm ("strcasestr") __attribute_pure__ __nonnull ((1, 2));
# else
extern char *strcasestr (const char *__haystack, const char *__needle)
     __THROW __attribute_pure__ __nonnull ((1, 2));
# endif
#endif

Both still use char* for the return type, not drmemory's const char* (nit: c++ has both).

What do you want to do?

@derekbruening
Copy link
Contributor

Looks like the supplied strcasestr implementation is for Windows. It must have been missing from Linux somewhere? Though if it was there in 1999 that seems doubtful. Seems like making the supplied version Windows-only would be fine.

#2429 has some overlap here. There /usr/include/string.h has a const char* return type with C++ linkage, vs the C linkage. In the glibc 2.40 paste above the const char* overload is still there.

If nothing else but the C++ overload returns const then removing the const from the utils_shared version seems fine too.

@xdje42
Copy link
Contributor Author

xdje42 commented Oct 4, 2024

data point: I found this comment in utils_shared.c:

/* Not available in ntdll CRT so we supply our own.
 * It is available on Mac and Android, and we want to avoid it for libs that do not
 * want a libc dependence.
 */
#if !defined(MACOS) && !defined(ANDROID) && !defined(NOLINK_STRCASESTR)
const char *
strcasestr(const char *text, const char *pattern)
{

So it seems we do need to define this for linux for libraries "that do not want a libc dependence".
Still should be able to just remove the const in the return type.

xdje42 added a commit that referenced this issue Oct 4, 2024
The definition of strcasestr is

char *strcasestr(const char *text, const char *pattern);

but drmemory uses

const char *strcasestr(const char *text, const char *pattern);

This causes build errors when both /usr/include/string.h and common/utils.h
are included in linux. The definition for strcasestr for C has always not
had the `const` in the result type (going back to its original addition
to glibc in 1997). Thus we're pretty safe in not breaking anything on *nix.
The other main use case of drmemory's strcasestr is on Windows, which does
not have strcasestr.

Tested:
$ cmake && make

Fixes #2522
@xdje42
Copy link
Contributor Author

xdje42 commented Oct 5, 2024

Open question: Why do current linux builds succeed? Unable to repro the success by replicating by hand, eg, ci-x86.yml.

@derekbruening
Copy link
Contributor

Open question: Why do current linux builds succeed? Unable to repro the success by replicating by hand, eg, ci-x86.yml.

I would assume the GA VM compiler version doesn't give a warning while your local version does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants