<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Oct 4, 2016, at 6:16 PM, Mehdi Amini via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class=""><div class=""><br class="Apple-interchange-newline">On Oct 4, 2016, at 6:11 PM, Zachary Turner <<a href="mailto:zturner@google.com" class="">zturner@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="" style="white-space: pre-wrap;">One possible way to make this actually be *safe* is to privately inherit ZeroStringRef from StringRef and bring in all the base class functions via using declarations. EXCEPT those base class functions that CANNOT un null terminate a null terminated string (eg drop_front() etc). For those functions, reimplement them to return a ZeroStringRef. <br class=""><br class="">Allow StringRef to be implicitly constructed from ZSR but not the other way around <br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">Yeah, that may be the best way forward. Also removing the StringRef constructor from a "const char *” at the same time.</div><div class=""><br class=""></div><div class="">I’ll try this after I’m done with my queue of StringRef conversions.</div></div></div></blockquote><div><br class=""></div><div>I’m not sure how it’ll look like on the API side though. Would a llvm::Value setName() take a StringRef or a ZSR?</div><div>What about StringMapEntry?</div><div><br class=""></div><div>A “generic” StringMap API would accept a StringRef, but on the other hand it’d lose some information when retrieving data from it. And it may be the common case (i.e. I suspect that most used of StringMap have null-terminated strings) that would be penalized.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class=""><br class=""></div><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class="" style="white-space: pre-wrap;"><br class="">Anyway, I don't feel qualified to lgtm a change like this so +chandlerc who is the owner of Support<br class=""><br class=""></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Tue, Oct 4, 2016 at 6:01 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" class="">dexonsmith@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">I disagree, although not strongly.<br class="gmail_msg"><br class="gmail_msg">Using data() and size() together is the expected usage model for StringRef. This matches many array-wrappers, and it's common practice to have data() be a raw pointer with size() indicating how large it is (see std::vector, SmallVector, ArrayRef, std::string, maybe others?).<br class="gmail_msg"><br class="gmail_msg">The common practice for cstr() is that it (a) is always valid and (b) returns a valid (and the correct) null-terminated string. StringRef::cstr has special semantics and different guarantees than the other cstr() I'm aware of. As a newcomer to LLVM, if I saw a StringRef::cstr, I would blindly assume it was valid and use it. The bugs would be fairly surprising.<br class="gmail_msg"><br class="gmail_msg">I'm also influenced by the point (I can't find it now) that ASan and the assert will not catch all the bugs with it, since the StringRef-ized char array could be in a struct (or a stream) that just happens to have a 0 next to it.<br class="gmail_msg"><br class="gmail_msg">Maybe there's a more neutral name than StringRef::unsafe_cstr (which sounds like it should never be called) or StringRef::cstr (which sounds like it magically does "the right thing").<br class="gmail_msg"><br class="gmail_msg">--<br class="gmail_msg"><br class="gmail_msg">Also, I just noticed Zachary is suggesting making this a local static function instead of StringRef API. I don't really have an opinion on that part... depends whether we'd (eventually) want to use this in more than one translation unit. If so, it should be StringRef API.<br class="gmail_msg"><br class="gmail_msg">> On 2016-Oct-04, at 16:57, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="gmail_msg" target="_blank">mehdi.amini@apple.com</a>> wrote:<br class="gmail_msg">><br class="gmail_msg">> Yet data() isn’t called “unsafe_data()” and is more unsafe than this one.<br class="gmail_msg">><br class="gmail_msg">>> On Oct 4, 2016, at 4:53 PM, Zachary Turner <<a href="mailto:zturner@google.com" class="gmail_msg" target="_blank">zturner@google.com</a>> wrote:<br class="gmail_msg">>><br class="gmail_msg">>> I'd be more comfortable if the name was like unsafe_cstr() or something. Thoughts?<br class="gmail_msg">>> On Tue, Oct 4, 2016 at 4:51 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="gmail_msg" target="_blank">mehdi.amini@apple.com</a>> wrote:<br class="gmail_msg">>>> On Oct 4, 2016, at 4:46 PM, Zachary Turner <<a href="mailto:zturner@google.com" class="gmail_msg" target="_blank">zturner@google.com</a>> wrote:<br class="gmail_msg">>>><br class="gmail_msg">>>> I mean "how many callsites are there”?<br class="gmail_msg">>><br class="gmail_msg">>> I haven’t audited / counted, and I’m not done with all the conversions yet.<br class="gmail_msg">>><br class="gmail_msg">>><br class="gmail_msg">>>><br class="gmail_msg">>>> On Tue, Oct 4, 2016 at 4:23 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="gmail_msg" target="_blank">mehdi.amini@apple.com</a>> wrote:<br class="gmail_msg">>>>> On Oct 4, 2016, at 4:17 PM, Zachary Turner <<a href="mailto:zturner@google.com" class="gmail_msg" target="_blank">zturner@google.com</a>> wrote:<br class="gmail_msg">>>>><br class="gmail_msg">>>>> How frequently would it need to be done at each callsite?<br class="gmail_msg">>>><br class="gmail_msg">>>> I don’t understand the question. Basically “always”?<br class="gmail_msg">>>><br class="gmail_msg">>>><br class="gmail_msg">>>>> My concern is just that it sounds like we're adding a potentially dangerous function to a very generic and widely used API for the purposes of one client.<br class="gmail_msg">>>><br class="gmail_msg">>>> Well the alternative to this API is .data(), the goal is to add a safer API than .data() :)<br class="gmail_msg">>>><br class="gmail_msg">>>> I'd inspect closely any client that is using .data() instead of this one.<br class="gmail_msg">>>><br class="gmail_msg">>>><br class="gmail_msg">>>><br class="gmail_msg">>>>> If you make the function available, people are going to use it without understanding the implications. Another possibility is that if this all takes place within a single common translation unit, you could write a private function inside there that looks like this:<br class="gmail_msg">>>>><br class="gmail_msg">>>>> const char *Unsafe_cstr(StringRef S) {<br class="gmail_msg">>>>> assert(strlen(S.data()) == S.size());<br class="gmail_msg">>>>> return S.data();<br class="gmail_msg">>>>> }<br class="gmail_msg">>>>><br class="gmail_msg">>>>> That way the unsafe function is not exposed to general clients of StringRef?<br class="gmail_msg">>>>><br class="gmail_msg">>>>> On Tue, Oct 4, 2016 at 4:10 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="gmail_msg" target="_blank">mehdi.amini@apple.com</a>> wrote:<br class="gmail_msg">>>>> The use case is that MachineInstruction have operands, but the size of their operands is limited. Right now you have the space for one-pointer, but not for a StringRef. So if you create a MachineInstruction that takes a symbol name as an operand, we can only store the “const chat *” from the StringRef.<br class="gmail_msg">>>>><br class="gmail_msg">>>>> So what you’re suggesting to do is roughly what I’m trying to achieve here as well, but instead of replicating the logic at each call sites, I try to abstract it behind the API.<br class="gmail_msg">>>>><br class="gmail_msg">>>>><br class="gmail_msg">>>>>> On Oct 4, 2016, at 3:56 PM, Zachary Turner <<a href="mailto:zturner@google.com" class="gmail_msg" target="_blank">zturner@google.com</a>> wrote:<br class="gmail_msg">>>>>><br class="gmail_msg">>>>>> I'm not familiar with those cases, but could you do something like this:<br class="gmail_msg">>>>>><br class="gmail_msg">>>>>> assert(strlen(s.data()) == s.size() && "Str is not null terminated!");<br class="gmail_msg">>>>>><br class="gmail_msg">>>>>> at each call site?<br class="gmail_msg">>>>>><br class="gmail_msg">>>>>> On Tue, Oct 4, 2016 at 3:48 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="gmail_msg" target="_blank">mehdi.amini@apple.com</a>> wrote:<br class="gmail_msg">>>>>> Neat, I didn’t know this one!<br class="gmail_msg">>>>>><br class="gmail_msg">>>>>> That does not solve cases like the round-trip StringRef -> MO -> StringRef though.<br class="gmail_msg">>>>>><br class="gmail_msg">>>>>><br class="gmail_msg">>>>>>> On Oct 4, 2016, at 3:38 PM, Zachary Turner <<a href="mailto:zturner@google.com" class="gmail_msg" target="_blank">zturner@google.com</a>> wrote:<br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>> Can you use %*s format specifier in those cases?<br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>> printf("%*s", s.size(), s.data());<br class="gmail_msg">>>>>>> On Tue, Oct 4, 2016 at 3:34 PM Mehdi AMINI <<a href="mailto:mehdi.amini@apple.com" class="gmail_msg" target="_blank">mehdi.amini@apple.com</a>> wrote:<br class="gmail_msg">>>>>>> mehdi_amini added a comment.<br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>> In<span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D25255#561566" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25255#561566</a>, @zturner wrote:<br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>> > Not sure how I feel about this. It's convenient, but it has potential for abuse. Where did you run into issues porting code to `StringRef` that this solves?<br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>> I ran into this with "printf" like API at some point, or with places that can't be converted to StringRef because of space constraint, like MachineOperand. So you're out of the IR and you have to use .data() to initialize the MO. Later you may construct a StringRef from this "const char *" again.<br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>> > I've done a lot of `StringRef` porting in LLDB by now, and I've always managed to find a solution to this. Usually it involves trickling the `StringRef` changes down further<br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>> Usually that's what I do, yes. The problem is when you reach some cases like above.<br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>><span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D25255" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25255</a><br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>>><br class="gmail_msg">>>>>><br class="gmail_msg">>>>><br class="gmail_msg">><br class="gmail_msg"><br class="gmail_msg"></blockquote></div></div></blockquote></div><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">llvm-commits mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:llvm-commits@lists.llvm.org" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">llvm-commits@lists.llvm.org</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div></blockquote></div><br class=""></body></html>