{"thread":{"id":"37639","subject":"[PATCH] Do not make trace.c/getnanotime an inlined function","startedAt":"2014-09-28T07:50:26Z","lastAt":"2014-09-30T15:54:49Z","messageCount":7,"participants":["Ben Walton","Johannes Sixt","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249961","messageId":"1411890626-28237-1-git-send-email-bdwalton@gmail.com","threadId":"37639","inReplyTo":null,"subject":"[PATCH] Do not make trace.c/getnanotime an inlined function","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2014-09-28T07:50:26Z","receivedAt":"2014-09-28T07:50:26Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Oracle Studio compilers don't allow for static variables in functions\nthat are defined to be inline. GNU C does permit this. Let's reference\nthe C99 standard though, which doesn't allow for inline functions to\ncontain modifiable static variables.\n\nSigned-off-by: Ben Walton \n---\n trace.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/trace.c b/trace.c\nindex b6f25a2..4778608 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -385,7 +385,7 @@ static inline uint64_t gettimeofday_nanos(void)\n * Returns nanoseconds since the epoch (01/01/1970), for performance tracing\n * (i.e. favoring high precision over wall clock time accuracy).\n */\n-inline uint64_t getnanotime(void)\n+uint64_t getnanotime(void)\n {\n \tstatic uint64_t offset;\n \tif (offset > 1) {\n-- \n1.9.1\n"},{"id":"249980","messageId":"54285E51.3090209@kdbg.org","threadId":"37639","inReplyTo":"1411890626-28237-1-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH] Do not make trace.c/getnanotime an inlined function","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-09-28T19:15:29Z","receivedAt":"2014-09-28T19:15:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 28.09.2014 um 09:50 schrieb Ben Walton:\n> Oracle Studio compilers don't allow for static variables in functions\n> that are defined to be inline. GNU C does permit this. Let's reference\n> the C99 standard though, which doesn't allow for inline functions to\n> contain modifiable static variables.\n> \n> Signed-off-by: Ben Walton \n> ---\n> trace.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/trace.c b/trace.c\n> index b6f25a2..4778608 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -385,7 +385,7 @@ static inline uint64_t gettimeofday_nanos(void)\n> * Returns nanoseconds since the epoch (01/01/1970), for performance tracing\n> * (i.e. favoring high precision over wall clock time accuracy).\n> */\n> -inline uint64_t getnanotime(void)\n> +uint64_t getnanotime(void)\n> {\n> \tstatic uint64_t offset;\n> \tif (offset > 1) {\n> \n\nBut then the function could stay static, no?\n\n-- Hannes\n"},{"id":"249992","messageId":"CACsJy8ArOU7WF4fiy5vn8zq5y6Vm5JxgTf+Tiai_WOeMSj--Ug@mail.gmail.com","threadId":"37639","inReplyTo":"1411890626-28237-1-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH] Do not make trace.c/getnanotime an inlined function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-09-29T12:44:20Z","receivedAt":"2014-09-29T12:44:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Sep 28, 2014 at 2:50 PM, Ben Walton wrote:\n> Oracle Studio compilers don't allow for static variables in functions\n> that are defined to be inline. GNU C does permit this. Let's reference\n> the C99 standard though, which doesn't allow for inline functions to\n> contain modifiable static variables.\n>\n> Signed-off-by: Ben Walton \n> ---\n> trace.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/trace.c b/trace.c\n> index b6f25a2..4778608 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -385,7 +385,7 @@ static inline uint64_t gettimeofday_nanos(void)\n> * Returns nanoseconds since the epoch (01/01/1970), for performance tracing\n> * (i.e. favoring high precision over wall clock time accuracy).\n> */\n> -inline uint64_t getnanotime(void)\n> +uint64_t getnanotime(void)\n> {\n> static uint64_t offset;\n\nWould moving this offset outside getnanotime() work?\n-- \nDuy\n"},{"id":"250000","messageId":"xmqqa95iuxlf.fsf@gitster.dls.corp.google.com","threadId":"37639","inReplyTo":"CACsJy8ArOU7WF4fiy5vn8zq5y6Vm5JxgTf+Tiai_WOeMSj--Ug@mail.gmail.com","subject":"Re: [PATCH] Do not make trace.c/getnanotime an inlined function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-29T17:48:28Z","receivedAt":"2014-09-29T17:48:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sun, Sep 28, 2014 at 2:50 PM, Ben Walton wrote:\n>> Oracle Studio compilers don't allow for static variables in functions\n>> that are defined to be inline. GNU C does permit this. Let's reference\n>> the C99 standard though, which doesn't allow for inline functions to\n>> contain modifiable static variables.\n>>\n>> Signed-off-by: Ben Walton \n>> ---\n>> trace.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/trace.c b/trace.c\n>> index b6f25a2..4778608 100644\n>> --- a/trace.c\n>> +++ b/trace.c\n>> @@ -385,7 +385,7 @@ static inline uint64_t gettimeofday_nanos(void)\n>> * Returns nanoseconds since the epoch (01/01/1970), for performance tracing\n>> * (i.e. favoring high precision over wall clock time accuracy).\n>> */\n>> -inline uint64_t getnanotime(void)\n>> +uint64_t getnanotime(void)\n>> {\n>> static uint64_t offset;\n>\n> Would moving this offset outside getnanotime() work?\n\nI am not sure what the definition of \"work\" is.\n\nThe function computes the difference between the returned value from\ngettimeofday(2) and a custom highres_nanos() just once and returns\nthe value it got from gettimeofday the first time, and then for\nsubsequent calls massages the returned value from highres_nanos() to\nbe consistent with the value returned from gettimeofday using the\noffset it computed in the first call.\n\nIf we have two copies of this function, two independent probes to\nthese pair of underlying functions will be made to compute their\noffsets. With perfect pair of clocks that may not matter, but it\njust feels wrong to me.\n\nBesides, I wonder what happens if the computed offset happen to be\n1, which is used as a sentinel.\n"},{"id":"250005","messageId":"5429C3B5.5020401@kdbg.org","threadId":"37639","inReplyTo":"CAP30j14QGtHC7huU=3t4sJT_dZ3t9V=CBWyGyJW7EjT9H5ZK9w@mail.gmail.com","subject":"Re: [PATCH] Do not make trace.c/getnanotime an inlined function","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-09-29T20:40:21Z","receivedAt":"2014-09-29T20:40:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 28.09.2014 um 22:42 schrieb Ben Walton:\n> On Sun, Sep 28, 2014 at 8:15 PM, Johannes Sixt > wrote:\n> Am 28.09.2014 um 09:50 schrieb Ben Walton:\n> > -inline uint64_t getnanotime(void)\n> > +uint64_t getnanotime(void)\n> \n> But then the function could stay static, no?\n> \n> \n> This function is used in several places outside of the translation unit\n> so while it's possible, I think it's more work than it's worth...\n\nI see. I didn't check myself, sorry. I assumed that due to the 'inline'\nthe function would not have been available outside. Now your patch looks\ngood.\n\nThanks,\n-- Hannes\n"},{"id":"250029","messageId":"CACsJy8Cnx=KQ02MT354Ly=o04=smbOhnrgCXLNa_tAtOPGmSdA@mail.gmail.com","threadId":"37639","inReplyTo":"xmqqa95iuxlf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Do not make trace.c/getnanotime an inlined function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-09-30T09:25:44Z","receivedAt":"2014-09-30T09:25:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Sep 30, 2014 at 12:48 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> On Sun, Sep 28, 2014 at 2:50 PM, Ben Walton wrote:\n>>> Oracle Studio compilers don't allow for static variables in functions\n>>> that are defined to be inline. GNU C does permit this. Let's reference\n>>> the C99 standard though, which doesn't allow for inline functions to\n>>> contain modifiable static variables.\n>>>\n>>> Signed-off-by: Ben Walton \n>>> ---\n>>> trace.c | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/trace.c b/trace.c\n>>> index b6f25a2..4778608 100644\n>>> --- a/trace.c\n>>> +++ b/trace.c\n>>> @@ -385,7 +385,7 @@ static inline uint64_t gettimeofday_nanos(void)\n>>> * Returns nanoseconds since the epoch (01/01/1970), for performance tracing\n>>> * (i.e. favoring high precision over wall clock time accuracy).\n>>> */\n>>> -inline uint64_t getnanotime(void)\n>>> +uint64_t getnanotime(void)\n>>> {\n>>> static uint64_t offset;\n>>\n>> Would moving this offset outside getnanotime() work?\n>\n> I am not sure what the definition of \"work\" is.\n>\n> The function computes the difference between the returned value from\n> gettimeofday(2) and a custom highres_nanos() just once and returns\n> the value it got from gettimeofday the first time, and then for\n> subsequent calls massages the returned value from highres_nanos() to\n> be consistent with the value returned from gettimeofday using the\n> offset it computed in the first call.\n>\n> If we have two copies of this function, two independent probes to\n> these pair of underlying functions will be made to compute their\n> offsets.\n\nHmm.. no. Even if the function is inlined in multiple places, inline\ncode still points to the same \"offset\" variable. So the\ngettimeofday_nanos()/highres_nanos() pair should only be called once.\nTested with gcc.\n-- \nDuy\n"},{"id":"250037","messageId":"xmqq7g0lt86u.fsf@gitster.dls.corp.google.com","threadId":"37639","inReplyTo":"CACsJy8Cnx=KQ02MT354Ly=o04=smbOhnrgCXLNa_tAtOPGmSdA@mail.gmail.com","subject":"Re: [PATCH] Do not make trace.c/getnanotime an inlined function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-30T15:54:49Z","receivedAt":"2014-09-30T15:54:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Hmm.. no. Even if the function is inlined in multiple places, inline\n> code still points to the same \"offset\" variable.\n\nOK, thanks.\n"}]}