- Download Diff

Summary:

5855 fix __RESTRICT_KYWD to support the correct keyword with c++

Review Request #36 — Created April 17, 2015 and submitted Oct. 19, 2015, 3:07 a.m.

Information
Owner:	risto3
Repository:	illumos-gate
Branch:
Bugs:	5855
Depends On:
Commit:	51b86d3...
Reviewers
Groups:	general
People:

Description

<added 19/04/2015>
I believe the description should reflect now the fact that these are rectifications
to what gnu C does in fix-includes for c++ purposes. This permits the same with
non g++/clang++ compilers such as sunstudio CC.

Perhaps:
5855 update posix_spawn[p] with rectified __RESTRICT_KYWD enabling 'restrict' semantics for both C and C++

Testing Done

The gate (on illumos-omnios) builds clean, lint-clean and shadow-compiler clean.

I've tested with both Sun C++ 5.9 SunOS_i386 Patch 124864-12 2009/04/21
and Sun C++ 5.10 SunOS_i386 128229-02 2009/09/21

I've also bootstrapped gcc (primarily 4.9.x now without my patch) and it swallows the good version just fine
as expected.

[update 2/07/2015]
I was mistaken, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61649 still needs to commit otherwise the
#ifdef __cplusplus
...
clause is doubled causing warnings, so I've asked again for a commit of the patches provided to gcc.

Issues

1
0
1
1
All Issues: 3

Description	From	Last Updated
Do C++ compilers define _STDC_C99 ?? This doesn't look like should work, unless they do, and if they do that ...	gdamore	April 17, 2015, 4:01 p.m.
I'd remove the space between # and define - to be consistent.	jeffpc	Sept. 2, 2015, 3:21 p.m.
Oh, I see... I thought that was talking about the presence of the #define not the specific whitespacing. Since the ...	jeffpc	Sept. 5, 2015, 6:26 a.m.

Fix it!

usr/src/uts/common/sys/feature_tests.h (Diff revision 1)

Show all issues

Do C++ compilers define _STDC_C99 ?? This doesn't look like should work, unless they do, and if they do that would be surprising.

I'm highly skeptical that this is the right behavior. It doesn't look like it should be harmful, assuming that a C++ compiler declares itself to support C99 and actually supports the restrict keyword.

The bug needs to have a clearer description of what problem you are actually trying to solve here. There are references to multiple compilers, and "fixed behavior", but what is the wrong behavior, and what is the correct behavior?

richlowe April 17, 2015, 4:27 p.m.

They do (erroneously), make that define.  We've had to fix bullshit related to that before.  There's history here in #2941
The other problem we've had with C++ and restrict is that the placement of restrict is differently standardized between C and C++.  This is what screwed up the standards prototype of posix_spawn.

       int posix_spawn(pid_t *restrict pid, const char *restrict path,
            const posix_spawn_file_actions_t *file_actions,
            const posix_spawnattr_t *restrict attrp,
            char *const argv[restrict], char *const envp[restrict]);


C++ doesn't allow that definition of argv and envp.

I'm somewhat worried this change will re-introduce that problem.

risto3 April 17, 2015, 5:18 p.m.

Indeed!  I found https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49347

which is probably why I don't see the issue with gcc (using the either patch with the solaris headers).
BTW, I find NetBSD did the following:

/

 * Spawn routines



/

int posix_spawn(pid_t * __restrict, const char * __restrict,

    const posix_spawn_file_actions_t , const posix_spawnattr_t * __restrict,

    char * const __restrict, char * const __restrict);

int posix_spawnp(pid_t * __restrict, const char * __restrict,

    const posix_spawn_file_actions_t , const posix_spawnattr_t * __restrict,

    char * const __restrict, char * const *__restrict);
FreeBSD does:

/

 * Spawn routines



 * XXX both arrays should be __restrict, but this does not work when GCC

 * is invoked with -std=c99.

/

int posix_spawn(pid_t * __restrict, const char * __restrict,

    const posix_spawn_file_actions_t , const posix_spawnattr_t * __restrict,

    char * const [], char * const []);

int posix_spawnp(pid_t * __restrict, const char * __restrict,

    const posix_spawn_file_actions_t *, const posix_spawnattr_t * __restrict,

    char * const [], char * const []);
NetBSD's approach seems reasonable.  FreeBSD's would work as well...
Too bad the Open Group didn't consider the c++ issue.
Back to the biz, the gate (for me) seems to build crystal clean with shadow compiling on and with lint.

I guess I can only see if there are some [test] programs using, e.g. posix_spawn

risto3 April 17, 2015, 5:22 p.m.

shit, markdown... sorry. guess we can't edit the entry here, I meant to fix the references
and add that the tests are primarily for studio.

risto3 April 22, 2015, 6:32 a.m.

BTW, with the updated patch providing what fixincl initially did to modify spawn.h,
gcc correctly passes through to native now as expected.
Therefore, two headers initially fixed up by gcc are now clean natively for both these issues.
I don't do bulk builds, but am pushing 1K packages in building pkgsrc with no perceived issue.

Change Summary:

This update applies what gcc fixincludes does and what NetBSD did for the spawn.h header
and equivalent in the implementation. Clean builds and all, testing the same.

Commit:

-	85b9ce61f15e6ce87434bd151fa32499cd722b3c
+	faaba67efdc40d8cac91e50150e3af7d69af2696

Diff:

Revision 2 (+13 -9)

Show changes

	usr/src/head/spawn.h
	usr/src/lib/libc/port/threads/spawn.c
	usr/src/uts/common/sys/feature_tests.h

Description:

		5855 fix __RESTRICT_KYWD to support the correct keyword with c++
	+
	+	<added 19/04/2015>
	+	I believe the description should reflect now the fact that these are rectifications
	+	to what gnu C does in fix-includes for c++ purposes. This permits the same with
	+	non g++/clang++ compilers such as sunstudio CC.
	+
	+	Perhaps:
	+	5855 update posix_spawn[p] with rectified __RESTRICT_KYWD enabling 'restrict' semantics for both C and C++

Testing Done:

		The gate (on illumos-omnios) builds clean, lint-clean and shadow-compiler clean.

		I've tested with both Sun C++ 5.9 SunOS_i386 Patch 124864-12 2009/04/21
		and Sun C++ 5.10 SunOS_i386 128229-02 2009/09/21

		I've also bootstrapped gcc (primarily 4.9.x now without my patch) and it swallows the good version just fine
		as expected.
	+
	+	[update 2/07/2015]
	+	I was mistaken, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61649 still needs to commit otherwise the
	+	#ifdef __cplusplus
	+	...
	+	clause is doubled causing warnings, so I've asked again for a commit of the patches provided to gcc.

Ship it!

```
Ship It!
```

Change Summary:

Update with a workaround avoiding any upstream gcc fixincludes
as well as updating as appropriate the comment text.

Commit:

-	faaba67efdc40d8cac91e50150e3af7d69af2696
+	00d8479830e7515534d040bdd8f01e6ade93a010

Diff:

Revision 3 (+19 -13)

Show changes

	usr/src/head/spawn.h
	usr/src/lib/libc/port/threads/spawn.c
	usr/src/uts/common/sys/feature_tests.h

usr/src/uts/common/sys/feature_tests.h (Diff revision 3)

Show all issues

I'd remove the space between # and define - to be consistent.

risto3 Sept. 2, 2015, 2:58 p.m.

Hi Josef, as I mention in the block comment preceding, the space is needed to FOIL fixincludes when building gcc.

Otherwise it forks up the definition. CSTYLE is dealt with via the inline comment.
I proposed upstream gcc a patch to add a line of context but it falls on deaf ears
(my suspicion is that illumos is not considered in the same light as Solaris, unfortunately).

usr/src/uts/common/sys/feature_tests.h (Diff revision 3)

Show all issues

Oh, I see... I thought that was talking about the presence of the #define not the specific whitespacing. Since the whitespace is important, I think it wouldn't be the worst thing to make the comment more... eye catching. How about moving that sentence closer to the strange looking #define? E.g.,

...
#else
/*
 * NOTE: The whitespace between the # and define is significant.  It foils gcc's fixincludes from defining a dedundant 'restrict'.
 */
 *
/* CSTYLED */
# define _RESTRICT_KYWD restrict
#endif

Change Summary:

update to comments per Josef's suggestion

Commit:

-	00d8479830e7515534d040bdd8f01e6ade93a010
+	51b86d3a9fcbd88d7fb054171b38d33947317496

Diff:

Revision 4 (+22 -13)

Show changes

	usr/src/head/spawn.h
	usr/src/lib/libc/port/threads/spawn.c
	usr/src/uts/common/sys/feature_tests.h

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

commit 10a6478a64c986fe8b918521951c9068ed5588fe

You have a pending review.

Review Board 3.0.18

5855 fix __RESTRICT_KYWD to support the correct keyword with c++

Change Summary:

Commit:

Diff:

Description:

Testing Done:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)

Change Summary: