[PATCH] D131290: [llvm-objdump] Create name for fake sections

[Namhyung Kim via Phabricator via llvm-commits]

namhyung added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:795
+    // Create a section name based on the p_type and index.
+    SecNames.push_back(StringRef("PT_LOAD#" + std::to_string(Idx)));
+    FakeShdr.sh_name = StrTab.add(SecNames.back());
----------------
MaskRay wrote:
> jhenderson wrote:
> > I'm pretty sure this isn't going to work as intended in many cases: you create a StringRef to a std::string temporary, and stick that in SecNames. It's likely that the StringRef will be pointing at invalid memory immediately after its construction (it's likely just working because nothing has scribbled over the data in memory yet). Could you not just populate `SecNames` with the result of the `"PT_LOAD#" + std::to_string(Idx)` expression directly?
> > 
> > If the StringTableBuilder contains StringRefs to the strings, using a std::vector will potentially invalidate them, if it needs to increase its capacity, right? We probably want a different strategy here. Probably simplest is simply to give the vector an initial capacity equal to the total number of program headers. Given that there are rarely that many, this shouldn't have any significant impact on performance.
> Thanks for catching this. It is a code smell but it actually works: `"PT_LOAD#" + std::to_string(Idx)` returns a `std::string`. It works as the the lifetime of the temporary object `std::string` applies to the full-expression.
> 
> The following should be better:
> 
> ```
> SmallVector<std::string, 0> SecNames; 
> ...
> SecNames.push_back(("PT_LOAD#" + Twine(Idx)).str());
> 
> // An alternative is ("PT_LOAD#" + Twine(Idx)).toVector(Name); but a local `SmallString` variable is needed
> ```
> 
> 
> I'm pretty sure this isn't going to work as intended in many cases: you create a StringRef to a std::string temporary, and stick that in SecNames. It's likely that the StringRef will be pointing at invalid memory immediately after its construction (it's likely just working because nothing has scribbled over the data in memory yet).

I don't think so.  The StringRef was needed because the constructor of SmallString takes it only.  I wanted to pass std::string directly but it's not supported IIUC.  After it creates a SmallString, I don't use the StringRef for a temp string anymore.  Instead it uses the SmallString data in the vector, that's why I passed `SecNames.back()`.  I believe it's safe to refer the data even after vector is resized.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131290/new/

https://reviews.llvm.org/D131290