<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Aug 19, 2014, at 2:27 PM, Shankar Easwaran <<a href="mailto:shankare@codeaurora.org">shankare@codeaurora.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 8/19/2014 2:08 PM, <a class="moz-txt-link-abbreviated" href="mailto:kledzik@apple.com">kledzik@apple.com</a>
wrote:<br>
</div>
<blockquote cite="mid:8a8f3c2297025604ee93423fc158cecd@localhost.localdomain" type="cite">
<pre wrap="">================
Comment at: include/lld/Core/LinkingContext.h:230
@@ +229,3 @@
+ /// as adjusting the atom's scope.
+ virtual void notifySymbolTableAdd(const Atom *atom) const {
+ }
----------------
Shankar Kalpathi Easwaran wrote:
</pre>
<blockquote type="cite">
<pre wrap=""><a class="moz-txt-link-abbreviated" href="mailto:kledzik@apple.com">kledzik@apple.com</a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Rui Ueyama wrote:
</pre>
<blockquote type="cite">
<pre wrap="">This function signature made me a bit sad because it's intended to mutate a given atom despite it takes const Atom *. We may probably want to make the atom graph mutable in the first place, because it's no longer actually const because of the addition of this function, so that we don't have to sprinkle const_casts in many places. I don't like to see that change in this CL but it may need to be considered later.
</pre>
</blockquote>
<pre wrap="">Yes. For now the graph is 99.9% const, so a few const_cast<> keeps it closer to what we are trying to model. But if we have Passes that need to modify atoms, or lots of places need start modifying atoms, it will probably make sense to remove the const in the zillion places.
</pre>
</blockquote>
<pre wrap="">Various flavors can go over the atoms in pass and set the appropriate attributes right, Am I missing something ? Same with the below function as well.
</pre>
</blockquote>
<pre wrap="">Shankar, Passes are run after resolving is done. So a Pass cannot tell, for instance, that a DefineAtom replaced a weak SharedLibraryAtom (which for ELF means the DefinedAtom should be dynamicExport). These hooks give the flavor's LinkingContext a change to record such flavor specific info.
================
Comment at: lib/Core/DefinedAtom.cpp:86
@@ -83,1 +85,3 @@
+void DefinedAtom::setScope(Scope s) {
+ assert(s == scope() && "setScope() must be overridden to alter scope");
----------------
Shankar Kalpathi Easwaran wrote:
</pre>
<blockquote type="cite">
<pre wrap="">I think visibility and scope are being mixed. While scope needs to be a property of the atom, I am not convinced visibility has to be part of the Atom has well, as visibility is mostly user driven.
</pre>
</blockquote>
<pre wrap="">What do you mean by "visibility"? The lld model does not have a visibility concept.
The ELF file format started with just two levels of scope (STB_LOCAL and STB_GLOBAL). Later the middle level of scope (linkage unit) was invented, for ELF it is represented as STB_GLOBAL plus STV_HIDDEN on the side.</pre>
</blockquote>
<br>
ELF has the concept of bindiing and visibility. <br>
<br>
Binding can be STB_LOCAL, STB_GLOBAL, STB_WEAK which could be
modeled using scope.<br>
<br>
Visibility plays a role mainly in dynamic linking (executables and
dynamic libraries). Common examples of visibility are STV_DEFAULT.
There are other variants of visibility STV_HIDDEN, STV_PROTECTED,
STV_INTERNAL.<br>
<br>
<snip from ELF abi for convenience><br>
<br>
<tt>The ELF application binary interface (ABI) defines the
visibility of symbols. Generally, it defines four classes, but in
most cases, only two of them are more commonly used:</tt>
<ul class="ibm-bullet-list">
<li><code>STV_DEFAULT</code><tt> - Symbols defined with it will be
exported. In other words, it declares that symbols are visible
everywhere.</tt></li>
<li><code>STV_HIDDEN</code><tt> - Symbols defined with it will not
be exported and cannot be used from other objects.</tt></li>
<li><tt>STV_PROTECTED: The symbol is visible outside the current
executable or shared object, but it may not be overridden. In
other words, if a protected symbol in a shared library is
referenced by an other code in the shared library, the other
code will always reference the symbol in the shared library,
even if the executable defines a symbol with the same name.</tt></li>
<li><tt>STV_INTERNAL: The symbol is not accessible outside the
current executable or shared library.</tt></li>
</ul>
</end snip><br>
<br>
The main problem I see with the setScope function from when called
from a callback function is, the atom loses the original property of
how it appeared in the object file.<br></div></blockquote><div>Yes. That is the big model change here. Previously, the atoms were always read-only. Now we are saying Passes and the LinkingContext may modify the atoms as the linking progresses. The goal being that by the end of the link, the atoms reflect what the Writer needs. If we don’t allow this, then we need to invent some sidecar somewhere which tracks the extra information Writers need. </div><br><blockquote type="cite"><div text="#000000" bgcolor="#FFFFFF">
<br>
If we decide that the Atom is not needed to be exported, the linker
could set the visibility attribute to HIDDEN/PROTECTED, but still my
original question remains that the NativeAtoms will not model how it
originally appeared in the object file.</div></blockquote><br></div><div>Are you concerned that full ELF symbol bits cannot survive a round trip through the Atom model (nothing about his patch changes that)? Or that something about the NativeAtom format? Or that the original state of any atom is lost by adding setters? </div><div><br></div><div>-Nick</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></body></html>