share/mk/bsd.mkopt.mk
16 ↗	(On Diff #62045)	Perhaps "or the value of MK_BAR if both WITH_FOO and WITHOUT_FOO are not defined." would be clearer? Otherwise it looks fine to me.

Use otherwise

Harbormaster completed remote builds in B26496: Diff 62096.Sep 14 2019, 3:55 PM

stevek accepted this revision.Sep 16 2019, 12:12 AM

This revision is now accepted and ready to land.Sep 16 2019, 12:12 AM

Closed by commit rS352370: Document logic for __DEFAULT_DEPENDENT_OPTIONS (authored by sjg). · Explain WhySep 16 2019, 12:32 AM

This revision was automatically updated to reflect the committed changes.

sjg added a commit: rS352370: Document logic for __DEFAULT_DEPENDENT_OPTIONS.

sjg added a commit: rS352782: Document logic for __DEFAULT_DEPENDENT_OPTIONS.Sep 27 2019, 12:11 AM

Revision Contents
Changeset List

Path

Size

head/

share/

mk/

bsd.mkopt.mk

10 lines

Diff 62136

View Options

head/share/mk/bsd.mkopt.mk

Show First 20 Lines • Show All 550 Lines • ▼ Show 20 Lines	hardclock_sync(int cpu)
int *t;		int *t;
KASSERT(!CPU_ABSENT(cpu), ("Absent CPU %d", cpu));		KASSERT(!CPU_ABSENT(cpu), ("Absent CPU %d", cpu));
t = DPCPU_ID_PTR(cpu, pcputicks);		t = DPCPU_ID_PTR(cpu, pcputicks);

*t = ticks;		*t = ticks;
}		}

/*		/*
* Compute number of ticks in the specified amount of time.		* Compute number of ticks representing the specified amount of time.
*/		*/
int		int
		kibUnsubmitted Not Done Inline Actions There should be a space after comma, for all macro definitions. kib: There should be a space after comma, for all macro definitions.
tvtohz(struct timeval *tv)		tvtohz(struct timeval *tv)
{		{
unsigned long ticks;		int64_t retval;
long sec, usec;		int64_t usec;

/*		/* convert time to microseconds */
		kibUnsubmitted Done Inline Actions This still assumes that int == int32_t. It can be larger. kib: This still assumes that int == int32_t. It can be larger.
		hselaskyAuthorUnsubmitted Done Inline Actions Yes, it can be larger and smaller, but not for the platforms we support? I can add a _Static_assert(sizeof(int) == sizeof(int32_t), "Assumption about integer types not met."); hselasky: Yes, it can be larger and smaller, but not for the platforms we support? I can add a…
		kibUnsubmitted Done Inline Actions But then why do you need casts at all? If you so rely on compiler I do not see how casts change anything. I compiled the following program both with clang 15.0.2 and gcc 12.2.0 on amd64, and both compiler generated the `imull` instruction. #include <limits.h> extern int hz; int tvtohz(long xx) { if (xx > INT_MAX) return (INT_MAX); if (xx < 0) return (1); return (xx * hz); } kib: But then why do you need casts at all? If you so rely on compiler I do not see how casts change…
		kibUnsubmitted Done Inline Actions In fact, I think that uint64_t cast is not needed as well. The holistic test for tv_sec would be not `tv_sec > INT_MAX`, but `tv_sec > INT_MAX / hz`. The `INT_MAX / hz` value can be precomputed once in the place which sets hz, to avoid divide in each tvtohz() invocation. kib: In fact, I think that uint64_t cast is not needed as well. The holistic test for tv_sec would…
		hselaskyAuthorUnsubmitted Done Inline Actions New logic does not make assumptions about size of "int". hselasky: New logic does not make assumptions about size of "int".
* If the number of usecs in the whole seconds part of the time		usec = (int64_t)tv->tv_sec * 1000000LL + (int64_t)tv->tv_usec;
		kibUnsubmitted Done Inline Actions LL is not needed due to the cast on the left side of mul Second cast to int64 is indeed required But, isn't it more wise to have long variants of your stime* functions, and use long there (basically you assume that int64 is same or higher rank than long, which is true for our arches right now, but who knows) kib: LL is not needed due to the cast on the left side of mul Second cast to int64 is indeed…
		hselaskyAuthorUnsubmitted Done Inline Actions I don't want to use long, because the limit is then a variable, LONG_MAX / hz, which results in a division. I found it better to use comparison with INT_MAX . hselasky: I don't want to use long, because the limit is then a variable, LONG_MAX / hz, which results in…
		kibUnsubmitted Done Inline Actions The double-cast (time_t->int32->int64) together with earlier checks for INT_MAX assumes that int32 == int, which is true now but not necessary so in the future. Why do you need it at all? Why casting hz at all? Also, the summary claims that you switched the code to use 32bit mul, while in fact you force 64bit multiplication (unless compiler is too smart, which I doubt). kib: The double-cast (time_t->int32->int64) together with earlier checks for INT_MAX assumes that…
		hselaskyAuthorUnsubmitted Done Inline Actions You are right. I don't need to cast "hz". I'll update the patch. Both clang and gcc know the (int64_t)(int32_t) thing: volatile int a,b; a = 1 << 24; b = 1 << 24; printf("0x%llx\n", (int64_t)a * (int64_t)b); printf("0x%llx\n", (int64_t)a * b); Results in a single imull instruction when using -m32 or native 32-bit compilation! volatile int a,b; a = 1 << 24; 401dc6: c7 45 f8 00 00 00 01 movl $0x1000000,-0x8(%ebp) b = 1 << 24; 401dcd: c7 45 fc 00 00 00 01 movl $0x1000000,-0x4(%ebp) printf("0x%llx\n", (int64_t)a * (int64_t)b); 401dd4: 8b 45 f8 mov -0x8(%ebp),%eax 401dd7: f7 6d fc imull -0x4(%ebp) 401dda: 52 push %edx 401ddb: 50 push %eax 401ddc: 68 fd 03 40 00 push $0x4003fd 401de1: e8 ba 00 00 00 call 401ea0 <printf@plt> hselasky: You are right. I don't need to cast "hz". I'll update the patch. Both clang and gcc know the…
* difference fits in a long, then the total number of usecs will
		kibUnsubmitted Done Inline Actions This changes the visible behavior. Previous code allowed negative tv_usecs (although not quite correctly). kib: This changes the visible behavior. Previous code allowed negative tv_usecs (although not quite…
* fit in an unsigned long. Compute the total and convert it to		/* convert to ticks */
* ticks, rounding up and adding 1 to allow for the current tick		retval = __stime64_scale32_floor(usec, hz, 1000000);
* to expire. Rounding also depends on unsigned long arithmetic
* to avoid overflow.		/* range check */
*		if (__predict_false(retval < 0))
* Otherwise, if the number of ticks in the whole seconds part of		retval = 0;
* the time difference fits in a long, then convert the parts to		else if (__predict_false(retval > INT_MAX - 1))
* ticks separately and add, using similar rounding methods and		retval = INT_MAX - 1;
* overflow avoidance. This method would work in the previous
* case but it is slightly slower and assumes that hz is integral.		/* add one additional tick */
*		return ((int)retval + 1);
* Otherwise, round the time difference down to the maximum
* representable value.
*
* If ints have 32 bits, then the maximum value for any timeout in
* 10ms ticks is 248 days.
*/
sec = tv->tv_sec;
usec = tv->tv_usec;
if (usec < 0) {
sec--;
usec += 1000000;
}
if (sec < 0) {
#ifdef DIAGNOSTIC
if (usec > 0) {
sec++;
usec -= 1000000;
}
printf("tvotohz: negative time difference %ld sec %ld usec\n",
sec, usec);
#endif
ticks = 1;
} else if (sec <= LONG_MAX / 1000000)
ticks = howmany(sec * 1000000 + (unsigned long)usec, tick) + 1;
else if (sec <= LONG_MAX / hz)
ticks = sec * hz
+ howmany((unsigned long)usec, tick) + 1;
else
ticks = LONG_MAX;
if (ticks > INT_MAX)
ticks = INT_MAX;
return ((int)ticks);
}		}

/*		/*
* Start profiling on a process.		* Start profiling on a process.
*		*
* Kernel profiling passes proc0 which never exits and hence		* Kernel profiling passes proc0 which never exits and hence
* keeps the profile clock running constantly.		* keeps the profile clock running constantly.
		kibUnsubmitted Done Inline Actions This would break compilation on arches where one of the types is not long. Use explicit cast for tv_sec/tv_usec to long with %ld format. kib: This would break compilation on arches where one of the types is not long. Use explicit cast…
*/		*/
void		void
startprofclock(struct proc *p)		startprofclock(struct proc *p)
{		{

PROC_LOCK_ASSERT(p, MA_OWNED);		PROC_LOCK_ASSERT(p, MA_OWNED);
if (p->p_flag & P_STOPPROF)		if (p->p_flag & P_STOPPROF)
return;		return;
if ((p->p_flag & P_PROFIL) == 0) {		if ((p->p_flag & P_PROFIL) == 0) {
		kibUnsubmitted Done Inline Actions Move comment right above macro definition kib: Move comment right above macro definition
p->p_flag \|= P_PROFIL;		p->p_flag \|= P_PROFIL;
		kibUnsubmitted Done Inline Actions Outer int cast is not needed kib: Outer int cast is not needed
mtx_lock(&time_lock);		mtx_lock(&time_lock);
if (++profprocs == 1)		if (++profprocs == 1)
cpu_startprofclock();		cpu_startprofclock();
mtx_unlock(&time_lock);		mtx_unlock(&time_lock);
}		}
		kibUnsubmitted Done Inline Actions How can this work, hz is runtime value, _Static_assert is compile-time construct. kib: How can this work, hz is runtime value, _Static_assert is compile-time construct.
		hselaskyAuthorUnsubmitted Done Inline Actions The formula is linear. Consider it more a sanity check. hselasky: The formula is linear. Consider it more a sanity check.
		impUnsubmitted Done Inline Actions I was confused until I realized hz here != extern int hz. It's the arg to the macro, so this just makes sure we have a good range... Maybe change it? Or have a better name than TIME_ASSERT for the macro. IMHO, it should be out the function to reinforce that it's not code that executes. imp: I was confused until I realized hz here != extern int hz. It's the arg to the macro, so this…
}		}

/*		/*
* Stop profiling on a process.		* Stop profiling on a process.
*/		*/
void		void
stopprofclock(struct proc *p)		stopprofclock(struct proc *p)
{		{
▲ Show 20 Lines • Show All 221 Lines • Show Last 20 Lines

Document logic for __DEFAULT_DEPENDENT_OPTIONSClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 62136

head/share/mk/bsd.mkopt.mk

Document logic for __DEFAULT_DEPENDENT_OPTIONS
ClosedPublic
Actions

Revision Contents
Changeset List