> On Jan 4, 2017, at 12:06 PM, sri raj paul <[email protected]> wrote: > >> By the way, I want the caller of chpl_task_idTostring() to pass a buffer >> (and length) to put the string in > Any reason to pass the length? Api’s like sprintf do similar things with only > the buffer. The caller of chpl_task_idTostring() should ensure the buffer is > big enough.
Just safety. Even with CHPL_TASK_ID_STRING_MAX_LEN defined (and with an accurate value) I don’t think it’s reasonable for us to require the caller to provide a buffer that big. That value is just the one we guarantee is the most we’ll need. If the caller is building up a text message piece by piece by appending in a fixed buffer, which is a somewhat common thing to do, when chpl_task_idToString() is called there may not be CHPL_TASK_ID_STRING_MAX_LEN bytes left in the buffer. (And it may be hard for that caller to guarantee that there are, if there are other variably-sized pieces in the message.) As long as the passed length accurately reflects the space available, chpl_task_idToString() can use snprintf() to guarantee it doesn’t overflow the buffer. But if it doesn’t know the length then it has to use sprintf() and if for any reason the caller’s buffer isn’t large enough, it’ll overflow. greg > > Thank you > Sriraj > >> On Jan 4, 2017, at 11:24 PM, Greg Titus <[email protected]> wrote: >> >> Pulling this up to the top … >> >>> This approach sounds fine to me too. I'd probably call the methods >>> 'chpl_task_idToString' and 'chpl_task_idEquals' but chpl_task_sameIDs >>> is OK if Greg really wants it to be that way :) >> >> I considered chpl_task_idEquals and ended up not liking it because we’ll be >> passing two IDs in and it seemed more natural to call it chpl_task_idsEqual, >> which I also wasn’t that fond of. Any of the 3 will work, though — coder’s >> choice. :-) >> >> >>> Sriraj, can we rely on you to create a pull request making these >>> changes? (at least if nobody else chimes in…) >> >> I would envision a total of either 3 or 4 PRs: >> • add chpl_task_idEquals() (or whatever) and chpl_task_idToString() to >> the existing public tasking implementations >> • (optional) the massiveThreads folks add the above to their tasking >> implementation (but could also be included in the first PR) >> • add the above to the Cray-private muxed tasking implementation (has >> to be done by the Chapel core team) >> • change to using these two functions in qio, chplvis, and anything >> else that needs them >> >> The important thing is to make sure that last one doesn’t happen until all >> the ones before have, so that we don’t end up not being able to build the >> runtime in some configurations. >> >> By the way, I want the caller of chpl_task_idTostring() to pass a buffer >> (and length) to put the string in, instead of allocating it dynamically. I >> expect the string will rarely (never?) need to outlive the caller’s >> activation record, so let’s avoid the allocation overhead. This probably >> means we also need the implementations to define something like a >> CHPL_TASK_ID_STRING_MAX_LEN preprocessor value that callers can use to >> ensure their buffer is big enough for any legal task ID representation. >> >> greg >> >> >>> On Jan 4, 2017, at 10:27 AM, Michael Ferguson <[email protected]> wrote: >>> >>> Hi - >>> >>> On 1/4/17, 11:57 AM, "Greg Titus" <[email protected]> wrote: >>> >>>> Actually, given Sriraj’s latest response I’ve come around to the other >>>> point of view. >>>> >>>> Let’s separate the issues of the internal and the external task ID >>>> representation. Even if the internal representation is a plain 64-bit >>>> int defined independently from the implementations, defining a >>>> chpl_task_sameIDs() to compare task IDs is a good idea so that the rest >>>> of the code isn’t dependent on that internal type. That’s just good >>>> interface engineering practice. >>>> >>>> Then, no matter what the internal representation is, only the >>>> implementations can provide a reasonable chpl_task_idToString() >>>> conversion function to produce the external representation, because only >>>> the implementations can decode or unpack encoded internal IDs. For >>>> example, if the encoded form has 16 high-order bits of node ID and 48 >>>> low-order bits of task ID within node, if some ID has node==1 and >>>> task_within_node==2 clearly it’s better to print “1:2” rather than the >>>> raw value “281474976710658" or “0x100000002” an implementation >>>> independent convertor would produce. >>>> >>>> Given that we need chpl_task_sameIDs() and chpl_task_idToString() anyway, >>>> I believe we should continue to have the implementations define the task >>>> ID type, so they can use whatever is convenient for them, and then >>>> require them to provide those two functions (and adjust to using those in >>>> qio and chplvis and elsewhere as needed). >>> >>> This approach sounds fine to me too. I'd probably call the methods >>> 'chpl_task_idToString' and 'chpl_task_idEquals' but chpl_task_sameIDs >>> is OK if Greg really wants it to be that way :) >>> >>> Sriraj, can we rely on you to create a pull request making these >>> >>> changes? (at least if nobody else chimes in...) >>> >>> -michael >> > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ Chapel-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/chapel-developers
