[PATCH] D76684: [obj2yaml] - Refactor how we dump sections. NFCI.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with nit.



================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:119-120
   StringRef Name = *NameOrErr;
+  if (Name.empty())
+    return "";
+
----------------
grimar wrote:
> jhenderson wrote:
> > Could you explain this case, please, perhaps with a comment in the code?
> It is for `obj2yaml\elf-null-section.yaml`. We have the following YAML description there:
> 
> ```
> --- !ELF
> FileHeader:
>   Class:   ELFCLASS64
>   Data:    ELFDATA2LSB
>   Type:    ET_REL
>   Machine: EM_X86_64
> Sections:
>   - Type: SHT_PROGBITS
>     Name: .foo
>   - Type:         SHT_NULL
>     Name:         ''
>     Flags:        [ SHF_ALLOC ]
>     Link:         1
>     Info:         2
>     AddressAlign: 0x3
>     Size:         0x4
>     EntSize:      0x5
>     Address:      0x6
> ```
> 
> Note: SHT_NULL has empty name and it is the second `SHT_NULL` section in the object.
> (first one is one at index 0, created implicitly).
> 
> We expect that obj2yaml produce the following;
> 
> ```
> # SECOND-SEC-NEXT:   - Type:         SHT_NULL
> # SECOND-SEC-NEXT:     Flags:        [ SHF_ALLOC ]
> # SECOND-SEC-NEXT:     Address:      0x0000000000000006
> # SECOND-SEC-NEXT:     Link:         .foo
> # SECOND-SEC-NEXT:     AddressAlign: 0x0000000000000003
> # SECOND-SEC-NEXT:     EntSize:      0x0000000000000005
> # SECOND-SEC-NEXT:     Content:      '00000000'
> # SECOND-SEC-NEXT:     Info:         0x0000000000000002
> ```
> 
> But now since we started to dump `SHT_NULL` sections (we remove it later), obj2yaml produces:
> ```
> - Name: ' [1]'
> ```
> 
> But it doesn't make sense I think. We should not uniquify empty names. I see no reasons for that.
Seems reasonable for now. I doubt we'll have many (any?) cases where we want to reference multiple different sections with empty names.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:121
+  // name (sh_name == 0). It normally doesn't happen, but when we have this case
+  // it doesn't make sense to uniquify their names and add noise to an output.
+  if (Name.empty())
----------------
an output -> the output


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

https://reviews.llvm.org/D76684