<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 3, 2017 at 8:31 PM, Sean Silva via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">silvas added a comment.<br>
<br>
This change does two things (as you mention in the description):<br>
<br>
1. Adding -static-func-strip-dirname-<wbr>prefix which provides a way to have more control when `-static-func-full-module-<wbr>prefix=true` is specified.<br></blockquote><div><br></div><div>This is actually a more general form of -static-func-full-mdoule-prefix.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2. Changing the default of -static-func-full-module-<wbr>prefix to true.<br>
<br>
IIRC, -static-func-full-module-<wbr>prefix defaults to false because it caused issues when set to true (in fact, it was introduced to avoid these issues). The default value of -static-func-strip-dirname-<wbr>prefix introduced in this patch (i.e. 0) is effectively a no-op; so ignore 1. for now. This means that the net effect of this patch is that compilation will, by default, have a regression on the issue fixed by r275193 / <a href="http://reviews.llvm.org/D22028" rel="noreferrer" target="_blank">http://reviews.llvm.org/D22028</a><wbr>, which is not a good idea. I think that the default behavior (which is user-visible) should not be changed in this patch.<br></blockquote><div><br></div><div>I disagree. The original default behavior was to preserve the full path which was also user visible :)</div><div><br></div><div>The whole rationale for changing the current default is that it is generally not safe -- mainly problem #1 because of counter variables for static functions can not guaranteed to be unique when full path is stripped.  The ThinLTO issue is secondary (probably irrelevant here because of other bugs).</div><div><br></div><div><br></div><div>The issue addressed in D22028 is actually not common -- the source module paths should generally match in profile-gen and profile-use phases, so using internal option for that use case seems more reasonable to me.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Overall, it sounds like this approach of relying on users to tweak internal compiler options (-mllvm) to get correct behavior in their environment is not the kind of user experience we want to deliver (or the kind of implementation that we want to maintain). IIRC, when we added -static-func-full-module-<wbr>prefix, it was with the understanding that it was a simple hack for working around the larger issue of relying on the module name which we knew was not very robust. The further addition of the "InLTO" complicates things even further. It seems like a code smell that we do not have a Single Point Of Truth.<br>
<br>
I proposed a solution at one point <a href="https://groups.google.com/d/msg/llvm-dev/s_VZbFTWbVs/d0b4Zh80CgAJ" rel="noreferrer" target="_blank">https://groups.google.com/d/<wbr>msg/llvm-dev/s_VZbFTWbVs/<wbr>d0b4Zh80CgAJ</a> though it may no longer be applicable. It seems like ThinLTO already has to solve a problem of finding unique identifiers for all functions (even static), so we may want to piggy-back on that mechanism (this is just a high-level thought; haven't looked into the details).<br>
<br></blockquote><div><br></div><div>For LTO/ThinLTO,  we solved the issue by using meta data which uses getPGOFuncName as singe source of truth.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
So:<br>
<br>
- I specifically object to changing user-visible defaults in this patch. Those changes should be isolated, and I don't think we have justification to change those defaults anyway.<br></blockquote><div><br></div><div>See my reply about the safety issue of keeping the current default. </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- I'm slightly opposed to adding the -static-func-strip-dirname-<wbr>prefix flag, since it seems like a workaround (among others that have already piled up) for a more fundamental issue. This is a frog-in-boiling-water situation; if solving the fundamental issue would be a huge amount of work, then adding the new flag is probably fine for now, but we need to keep in mind the larger situation. IIUC, defaulting `-static-func-strip-dirname-<wbr>prefix=-1` would emulate the current default behavior, so -static-func-full-module-<wbr>prefix could just be removed in the same patch.<br></blockquote><div><br></div><div>The current -static-func-full-module-path=false is simply a special case of the new option. For users who rely on this option may hit the correctness issue, they won't have any fallback without the new option.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- I would encourage brainstorming/discussion of alternative solutions that solve the fundamental problem (which seems to be more about having a stable globally unique identifier than being specifically about preserving/mangling the "name" per se).<br></blockquote><div><br></div><div>The problem itself is simple: handle name conflicts between</div><div><br></div><div>/a/b/c/foo.c:static_func</div><div>/e/f/g/foo.c:static_func</div><div><br></div><div>Path info is a natural choice. Note that FE instrumentation also uses module path to uniquely identify static_func as well.</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<br>
<br>
<a href="https://reviews.llvm.org/D29512" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D29512</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>