<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 28, 2017 at 7:10 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-2401689961372690851gmail-m_5659932387484374208gmail-">> ================<br>
> Comment at: llvm/include/llvm/Object/IRSym<wbr>tab.h:31<br>
> +<br>
> +typedef support::ulittle32_t Word;<br>
> +<br>
> ----------------<br>
> rafael wrote:<br>
>> why do you use this for Symbol? If we are storing something by value it is better to just use a uint32_t.<br>
> If we eventually want to write these data structures out to files I guess we will need to specify an endianness. The plan I have in mind is:<br>
> 1. define data structures that can be potentially written to files, but use them only as an in-memory format to start with (that is what this patch does)<br>
> 2. potentially improve them in-tree (e.g. by sharing the string table between the module and the symbol table)<br>
> 3. flip the switch and start writing them to files<br>
<br>
</span>The two step solution is very reasonable. I see that you currently<br>
write to memory and read back, so it should be easy to write and read<br>
from disk, and yes, we have to specify the endianness for a disk<br>
format. But I think the endianness should be taken care of at the<br>
read/write functions. This is similar to ELF: </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
* The on disk format has an specific endianness.<br>
* A pointer to a mmap area should use something like support::ulittle32_t*.<br>
* But it should be read once producing a uint32_t.<br></blockquote><div><br></div><div>I agree that the pointers to the mmap area should use something low level that exposes the endianness, and that is what I intend the structs that I am introducing in this patch to be used for. Although the reader code could unpack the data structures manually out of a support::ulittle32_t* pointer, I think the structs help with readability.</div><div><br></div><div>Essentially the data structures I am adding are the equivalent of the definitions in llvm/Object/ELFTypes.h for ELF.</div><div><br></div><div>On top of that, I think it seems reasonable to build a more convenient API that reads data once and uses types like uint32_t, and it could even be part of this patch. I'll start working on that.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>From the work on lld I think a reasonable rule of the thumb is then<br>
that the endianness tagged types like support::ulittle32_t should only<br>
be used in pointers.<br></blockquote><div><br></div><div>I guess you're referring to for example how we read fields once from an Elf_Sym and copy them into a SymbolBody. If so, that makes perfect sense to me.</div><div><br></div><div>Thanks,</div></div>-- <br><div class="m_-2401689961372690851gmail-m_5659932387484374208gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</div></div>