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

[Namhyung Kim via Phabricator via llvm-commits]

namhyung added a comment.

In D131290#3771526 <https://reviews.llvm.org/D131290#3771526>, @jhenderson wrote:

> In D131290#3771470 <https://reviews.llvm.org/D131290#3771470>, @MaskRay wrote:
>
>> In D131290#3771459 <https://reviews.llvm.org/D131290#3771459>, @jhenderson wrote:
>>
>>> In D131290#3771385 <https://reviews.llvm.org/D131290#3771385>, @namhyung wrote:
>>>
>>>>   SmallVector<std::string, 0> SecNames; 
>>>>   ...
>>>>   SecNames.push_back(("PT_LOAD#" + Twine(Idx)).str());
>>>>
>>>> This makes the test failing again.
>>>
>>> Please see the second half of my previous comment re. vector resizing (can apply to SmallVector too potentially). I would expect that to be a problem.
>>
>> I think we should switch to what I originally suggested, drop StringTableBuilder and use a string with `\0` terminators:)
>
> I'm actually inclined to agree - the "simplification" is turning out to be more complicated than I expected.

Let me try one more time before going back.  I think this failure came from the std::string implementation.  From a quick glance it seems to store small strings inside the object, not in the external allocation.  Then a later move due to vector resize will invalidate earlier references.  But SmallString seems to store the data always in the external allocation so it'd be fine after move.  Actually it passes the test when I change it like below

  -  SmallVector<std::string, 0> SecNames;
  +  SmallVector<SmallString<10>, 0> SecNames;

Let me know if I missed something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131290