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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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());
----------------
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
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131290