<div dir="ltr">You submitted a patch without code review which introduced new notions such as "redirecting using a separate undefined atom" which was never discussed before. I'm still trying to get what that means and whether they are actually well designed or not.<div><br></div><div>So, is this change needed? If you need to maintain a relationship between references and symbols only in the reader, you can make a map from references to symbols and use that, instead of adding a new field to store symbols to references.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 23, 2015 at 10:42 AM, Shankar Easwaran <span dir="ltr"><<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Rui,<br>
<br>
Its not the name of the reference, We need the properties of the node from the reference.<br>
<br>
If the reference is from a node, which is in the same group as the reference is, we dont need to redirect using a undefined reference, otherwise it has to be routed through a undefined reference.<br>
<br>
Example :-<br>
<br>
If foo(node) calls bar(reference) which is in the same group, foo can directly call bar.<br>
<br>
If baz(node) which is in a different group tries to call bar(reference), it has to be redirected using a undefined reference.<span class="HOEnZb"><font color="#888888"><br>
<br>
Shankar Easwaran</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On 2/23/2015 12:31 PM, Rui Ueyama wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
My point is that usually when you want to know the name of a symbol to<br>
which a reference is pointing, you'd do ref->getTarget()->name() instead of<br>
adding a name to a reference. In LLD's model, symbol name is a property of<br>
the node (atom) and not a property of edge (reference).<br>
<br>
My question is that why you had to add a name to the reference, instead of<br>
using the usual way? What's special about the feature that you were trying<br>
to implement?<br>
<br>
On Mon, Feb 23, 2015 at 4:59 AM, Shankar Easwaran <<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>><br>
wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Rui,<br>
<br>
Thanks for the review.<br>
<br>
This is used to lookup the symbol/section, for a reference.<br>
<br>
This is used to check if the symbol/section that points to the reference<br>
is from outside the group (or) not in the same section group.<br>
<br>
More details are here FYI. <a href="https://mentorembedded.github" target="_blank">https://mentorembedded.github</a>.<br>
io/cxx-abi/abi/prop-72-comdat.<u></u>html<br>
<br>
Shankar Easwaran<br>
<br>
<br>
On 2/22/2015 9:34 PM, Rui Ueyama wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This patch looks really weird. Basically in the atom model a reference is<br>
a<br>
relocation, and an atom is a symbol + data. However, looks like this patch<br>
made a reference to have an item that it is pointing to. If so, that<br>
doesn't sound correct.<br>
<br>
Send this kind of nontrivial changes to pre-commit review first, please.<br>
<br>
On Sun, Feb 22, 2015 at 3:46 PM, Shankar Easwaran <<br>
<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>><br>
wrote:<br>
<br>
Author: shankare<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Date: Sun Feb 22 17:46:21 2015<br>
New Revision: 230191<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=230191&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=230191&view=rev</a><br>
Log:<br>
[ELF] Add symbol to ELFReference.<br>
<br>
Relocation handling need more information about the Symbol that we are<br>
creating<br>
references for.<br>
<br>
No change in functionality.<br>
<br>
Modified:<br>
lld/trunk/lib/ReaderWriter/<u></u>ELF/Atoms.h<br>
lld/trunk/lib/ReaderWriter/<u></u>ELF/ELFFile.h<br>
lld/trunk/lib/ReaderWriter/<u></u>ELF/Mips/MipsELFFile.h<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/<u></u>ELF/Atoms.h<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/lib/</a><br>
ReaderWriter/ELF/Atoms.h?rev=<u></u>230191&r1=230190&r2=230191&<u></u>view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/lib/ReaderWriter/<u></u>ELF/Atoms.h (original)<br>
+++ lld/trunk/lib/ReaderWriter/<u></u>ELF/Atoms.h Sun Feb 22 17:46:21 2015<br>
@@ -35,23 +35,27 @@ template <typename ELFT> class ELFFile;<br>
template <class ELFT> class ELFReference : public Reference {<br>
typedef llvm::object::Elf_Rel_Impl<<u></u>ELFT, false> Elf_Rel;<br>
typedef llvm::object::Elf_Rel_Impl<<u></u>ELFT, true> Elf_Rela;<br>
+ typedef llvm::object::Elf_Sym_Impl<<u></u>ELFT> Elf_Sym;<br>
+<br>
public:<br>
- ELFReference(const Elf_Rela *rela, uint64_t off, Reference::KindArch<br>
arch,<br>
- Reference::KindValue relocType, uint32_t idx)<br>
- : Reference(Reference::<u></u>KindNamespace::ELF, arch, relocType),<br>
+ ELFReference(const Elf_Sym *sym, const Elf_Rela *rela, uint64_t off,<br>
+ Reference::KindArch arch, Reference::KindValue relocType,<br>
+ uint32_t idx)<br>
+ : Reference(Reference::<u></u>KindNamespace::ELF, arch, relocType),<br>
_sym(sym),<br>
_target(nullptr), _targetSymbolIndex(idx), _offsetInAtom(off),<br>
_addend(rela->r_addend) {}<br>
<br>
- ELFReference(uint64_t off, Reference::KindArch arch,<br>
+ ELFReference(const Elf_Sym *sym, uint64_t off, Reference::KindArch<br>
arch,<br>
Reference::KindValue relocType, uint32_t idx)<br>
- : Reference(Reference::<u></u>KindNamespace::ELF, arch, relocType),<br>
+ : Reference(Reference::<u></u>KindNamespace::ELF, arch, relocType),<br>
_sym(sym),<br>
_target(nullptr), _targetSymbolIndex(idx), _offsetInAtom(off),<br>
_addend(0) {}<br>
<br>
ELFReference(uint32_t edgeKind)<br>
: Reference(Reference::<u></u>KindNamespace::all,<br>
Reference::KindArch::all,<br>
edgeKind),<br>
- _target(nullptr), _targetSymbolIndex(0), _offsetInAtom(0),<br>
_addend(0) {}<br>
+ _sym(nullptr), _target(nullptr), _targetSymbolIndex(0),<br>
+ _offsetInAtom(0), _addend(0) {}<br>
<br>
uint64_t offsetInAtom() const override { return _offsetInAtom; }<br>
<br>
@@ -70,7 +74,10 @@ public:<br>
<br>
void setTarget(const Atom *newAtom) override { _target = newAtom; }<br>
<br>
+ const Elf_Sym *symbol() const { return _sym; }<br>
+<br>
private:<br>
+ const Elf_Sym *_sym;<br>
const Atom *_target;<br>
uint64_t _targetSymbolIndex;<br>
uint64_t _offsetInAtom;<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/<u></u>ELF/ELFFile.h<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/lib/</a><br>
ReaderWriter/ELF/ELFFile.h?<u></u>rev=230191&r1=230190&r2=<u></u>230191&view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/lib/ReaderWriter/<u></u>ELF/ELFFile.h (original)<br>
+++ lld/trunk/lib/ReaderWriter/<u></u>ELF/ELFFile.h Sun Feb 22 17:46:21 2015<br>
@@ -181,12 +181,12 @@ protected:<br>
std::error_code doParse() override;<br>
<br>
/// \brief Iterate over Elf_Rela relocations list and create<br>
references.<br>
- virtual void createRelocationReferences(<u></u>const Elf_Sym &symbol,<br>
+ virtual void createRelocationReferences(<u></u>const Elf_Sym *symbol,<br>
ArrayRef<uint8_t> content,<br>
range<Elf_Rela_Iter> rels);<br>
<br>
/// \brief Iterate over Elf_Rel relocations list and create<br>
references.<br>
- virtual void createRelocationReferences(<u></u>const Elf_Sym &symbol,<br>
+ virtual void createRelocationReferences(<u></u>const Elf_Sym *symbol,<br>
ArrayRef<uint8_t> symContent,<br>
ArrayRef<uint8_t> secContent,<br>
range<Elf_Rel_Iter> rels);<br>
@@ -796,12 +796,12 @@ ELFDefinedAtom<ELFT> *ELFFile<ELFT>::cre<br>
// Add Rela (those with r_addend) references:<br>
auto rari = _relocationAddendReferences.<u></u>find(sectionName);<br>
if (rari != _relocationAddendReferences.<u></u>end())<br>
- createRelocationReferences(*<u></u>symbol, symContent, rari->second);<br>
+ createRelocationReferences(<u></u>symbol, symContent, rari->second);<br>
<br>
// Add Rel references.<br>
auto rri = _relocationReferences.find(<u></u>sectionName);<br>
if (rri != _relocationReferences.end())<br>
- createRelocationReferences(*<u></u>symbol, symContent, secContent,<br>
rri->second);<br>
+ createRelocationReferences(<u></u>symbol, symContent, secContent,<br>
rri->second);<br>
<br>
// Create the DefinedAtom and add it to the list of DefinedAtoms.<br>
return *handleDefinedSymbol(<u></u>symbolName, sectionName, symbol,<br>
section,<br>
@@ -810,35 +810,35 @@ ELFDefinedAtom<ELFT> *ELFFile<ELFT>::cre<br>
}<br>
<br>
template <class ELFT><br>
-void ELFFile<ELFT>::<u></u>createRelocationReferences(<u></u>const Elf_Sym &symbol,<br>
+void ELFFile<ELFT>::<u></u>createRelocationReferences(<u></u>const Elf_Sym *symbol,<br>
ArrayRef<uint8_t><br>
content,<br>
range<Elf_Rela_Iter><br>
rels)<br>
{<br>
bool isMips64EL = _objFile->isMips64EL();<br>
- const auto symValue = getSymbolValue(&symbol);<br>
+ const auto symValue = getSymbolValue(symbol);<br>
for (const auto &rel : rels) {<br>
if (rel.r_offset < symValue ||<br>
symValue + content.size() <= rel.r_offset)<br>
continue;<br>
_references.push_back(new (_readerStorage) ELFReference<ELFT>(<br>
- &rel, rel.r_offset - symValue, kindArch(),<br>
+ symbol, &rel, rel.r_offset - symValue, kindArch(),<br>
rel.getType(isMips64EL), rel.getSymbol(isMips64EL)));<br>
}<br>
}<br>
<br>
template <class ELFT><br>
-void ELFFile<ELFT>::<u></u>createRelocationReferences(<u></u>const Elf_Sym &symbol,<br>
+void ELFFile<ELFT>::<u></u>createRelocationReferences(<u></u>const Elf_Sym *symbol,<br>
ArrayRef<uint8_t><br>
symContent,<br>
ArrayRef<uint8_t><br>
secContent,<br>
range<Elf_Rel_Iter><br>
rels) {<br>
bool isMips64EL = _objFile->isMips64EL();<br>
- const auto symValue = getSymbolValue(&symbol);<br>
+ const auto symValue = getSymbolValue(symbol);<br>
for (const auto &rel : rels) {<br>
if (rel.r_offset < symValue ||<br>
symValue + symContent.size() <= rel.r_offset)<br>
continue;<br>
_references.push_back(new (_readerStorage) ELFReference<ELFT>(<br>
- rel.r_offset - symValue, kindArch(),<br>
- rel.getType(isMips64EL), rel.getSymbol(isMips64EL)));<br>
+ symbol, rel.r_offset - symValue, kindArch(),<br>
rel.getType(isMips64EL),<br>
+ rel.getSymbol(isMips64EL)));<br>
int32_t addend = *(symContent.data() + rel.r_offset - symValue);<br>
_references.back()->setAddend(<u></u>addend);<br>
}<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/<u></u>ELF/Mips/MipsELFFile.h<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/Mips/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/lib/<u></u>ReaderWriter/ELF/Mips/</a><br>
MipsELFFile.h?rev=230191&r1=<u></u>230190&r2=230191&view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/lib/ReaderWriter/<u></u>ELF/Mips/MipsELFFile.h (original)<br>
+++ lld/trunk/lib/ReaderWriter/<u></u>ELF/Mips/MipsELFFile.h Sun Feb 22<br>
17:46:21<br>
2015<br>
@@ -199,17 +199,17 @@ private:<br>
return std::error_code();<br>
}<br>
<br>
- void createRelocationReferences(<u></u>const Elf_Sym &symbol,<br>
+ void createRelocationReferences(<u></u>const Elf_Sym *symbol,<br>
ArrayRef<uint8_t> symContent,<br>
ArrayRef<uint8_t> secContent,<br>
range<Elf_Rel_Iter> rels) override {<br>
for (Elf_Rel_Iter rit = rels.begin(), eit = rels.end(); rit != eit;<br>
++rit) {<br>
- if (rit->r_offset < symbol.st_value ||<br>
- symbol.st_value + symContent.size() <= rit->r_offset)<br>
+ if (rit->r_offset < symbol->st_value ||<br>
+ symbol->st_value + symContent.size() <= rit->r_offset)<br>
continue;<br>
<br>
this->_references.push_back(<u></u>new (this->_readerStorage)<br>
ELFReference<ELFT>(<br>
- rit->r_offset - symbol.st_value, this->kindArch(),<br>
+ symbol, rit->r_offset - symbol->st_value, this->kindArch(),<br>
rit->getType(isMips64EL()), rit->getSymbol(isMips64EL())))<u></u>;<br>
<br>
auto addend = getAddend(*rit, secContent);<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
<br>
<br>
</blockquote></blockquote>
--<br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted<br>
by the Linux Foundation<br>
<br>
<br>
</blockquote></blockquote>
<br>
<br>
-- <br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation<br>
<br>
</div></div></blockquote></div><br></div>