<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 8, 2016 at 11:12 AM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg added a comment.<br class="gmail_msg">
<br class="gmail_msg">
See inlined comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: ELF/GdbIndex.cpp:105-118<br class="gmail_msg">
+ DataExtractor PubNames(D, IsLE, 0);<br class="gmail_msg">
+ uint32_t Offset = 0;<br class="gmail_msg">
+ while (PubNames.isValidOffset(Offset)) {<br class="gmail_msg">
+ // Skip length, version, unit offset and size.<br class="gmail_msg">
+ Offset += 14;<br class="gmail_msg">
+ while (Offset < D.size()) {<br class="gmail_msg">
+ uint32_t DieRef = PubNames.getU32(&Offset);<br class="gmail_msg">
----------------<br class="gmail_msg">
Might be worth putting this pubnames/pubtypes parser into the llvm/lib/DebugInfo/DWARF. </blockquote><div><br>+1<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">We don't currently have a parser for this there.</blockquote><div><br></div><div>llvm-dwarfdump can dump {gnu_,}pubnames sections, so I think we do already have a parser there - we should use that one (may need some refactoring to be nice and general - maybe it's just parsing+dumping all bundled up together)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The more we duplicate code that is manually parsing DWARF sections outside of the DWARF parser the more fixes we need to make if the format gets updated.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: ELF/SyntheticSections.cpp:1417<br class="gmail_msg">
+// <a href="https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html" rel="noreferrer" class="gmail_msg" target="_blank">https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html</a><br class="gmail_msg">
+static uint32_t hash(const char *Str) {<br class="gmail_msg">
+ uint32_t R = 0;<br class="gmail_msg">
----------------<br class="gmail_msg">
Use StringRef here instead of "const char *"?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: ELF/SyntheticSections.cpp:1420<br class="gmail_msg">
+ uint8_t C;<br class="gmail_msg">
+ while ((C = *Str++) != 0) {<br class="gmail_msg">
+ C = tolower(C);<br class="gmail_msg">
----------------<br class="gmail_msg">
Use the StringRef iteration here?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: ELF/SyntheticSections.cpp:1429<br class="gmail_msg">
void GdbIndexSection<ELFT>::readDwarf(InputSection<ELFT> *I) {<br class="gmail_msg">
- std::vector<std::pair<uintX_t, uintX_t>> CuList = readCuList(I);<br class="gmail_msg">
+ assert(!Finalized);<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
Maybe add a:<br class="gmail_msg">
```<br class="gmail_msg">
if (Finalized)<br class="gmail_msg">
return;<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
So that this code works if asserts are not enabled? I would be great to reduce the amounts of places that use assert unconditionally in the llvm codebase.<br class="gmail_msg"></blockquote><div><br></div><div>That's not really idiomatic to the LLVM codebase - asserts represents invariants in the code. If those invariants are violated, the behavior of the program is undefined, untested, etc.<br><br>(it'd be like returning on every array index if the array index is out of bounds - we don't do that, we just take it as read that we as programmers must ensure the indexes are valid)<br><br>Asserts are debugging tools. If we added code to fail gracefully behind asserts, we wouldn't actually know if any of that worked (we'd never test it - because any time we hit an assert it'd be a bug we would fix) - it could just cause the code to go off in some other random direction instead of the return path.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br class="gmail_msg">
Comment at: ELF/SyntheticSections.cpp:1441<br class="gmail_msg">
+ Builder.readPubNamesAndTypes();<br class="gmail_msg">
+ for (std::pair<StringRef, uint8_t> &Pair : NamesAndTypes) {<br class="gmail_msg">
+ uint32_t Hash = hash(Pair.first.data());<br class="gmail_msg">
----------------<br class="gmail_msg">
grimar wrote:<br class="gmail_msg">
> clayborg wrote:<br class="gmail_msg">
> > use std::tie here with appropriate var names since you did it below?<br class="gmail_msg">
> I am not sure what you mean, sorry. Do you meant next form ?<br class="gmail_msg">
><br class="gmail_msg">
> for (auto& p : someInitializingFunction()) {<br class="gmail_msg">
> std::tie(a, b) = p;<br class="gmail_msg">
> // do stuff;<br class="gmail_msg">
> }<br class="gmail_msg">
><br class="gmail_msg">
> If so I am probably missing for what.<br class="gmail_msg">
Never mind, I was thinking you can use std::tie in the for loop:<br class="gmail_msg">
```<br class="gmail_msg">
for (std::tie(Name, Type) : NamesAndTypes) {<br class="gmail_msg">
```<br class="gmail_msg">
but you can't.<br class="gmail_msg"></blockquote><div><br></div><div>If you declare the variables outside the loop you could - but that's not always an improvement. C++17 I think maybe gets the ability to do a multi-declaration/tie thing.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: ELF/SyntheticSections.cpp:1442<br class="gmail_msg">
+ for (std::pair<StringRef, uint8_t> &Pair : NamesAndTypes) {<br class="gmail_msg">
+ uint32_t Hash = hash(Pair.first.data());<br class="gmail_msg">
+ size_t Offset = StringPool.add(Pair.first);<br class="gmail_msg">
----------------<br class="gmail_msg">
grimar wrote:<br class="gmail_msg">
> clayborg wrote:<br class="gmail_msg">
> > I am guessing we can guarantee that the StringRef is NULL terminated here since the string probably originated from a string table.<br class="gmail_msg">
> Yes, and hash() relies on that. Do you want to change hash() signature ?<br class="gmail_msg">
Might be safer to change the hash() signature if the only way we use it is with an llvm::StringRef.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: ELF/SyntheticSections.cpp:1522<br class="gmail_msg">
+ Buf += 4;<br class="gmail_msg">
+ for (std::pair<uint32_t, uint8_t> &P : CuVec) {<br class="gmail_msg">
+ uint32_t Index = P.first;<br class="gmail_msg">
----------------<br class="gmail_msg">
clayborg wrote:<br class="gmail_msg">
> use std::tie?<br class="gmail_msg">
Never mind, can't use std::tie in for loop<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D26283" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D26283</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>