[PATCH] D143841: [Propeller] Make decoding BBAddrMaps trace through relocations

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ELF.cpp:706
+      if (FOTIterator == FunctionOffsetTranslations.end())
+        return createError("Failed to get relocation data for offset: " +
+                           Twine(SectionOffset) + " in section " +
----------------
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:710
+  for (auto const &[Sec, RelocSec] : *SectionRelocMapOrErr) {
+    std::optional<Elf_Shdr> RelaSec = {};
+    if (IsRelocatable)
----------------
rahmanl wrote:
> `std::nullopt`.
As far as I understand it, you don't need either - `std::optional` has a default constructor to initialise as empty. In other words, this line should just be `std::optional<Elf_Shdr> RelaSec;`


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml:5
+
+# Fails on windows (https://github.com/llvm/llvm-project/issues/60013).
+# UNSUPPORTED: system-windows
----------------
jhenderson wrote:
> 
Not addressed still.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml:5
+
+# UNSUPPORTED: system-windows
+
----------------
rahmanl wrote:
> aidengrossman wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > Why is this unsupported on Windows?
> > > Given we have an open ticket for this, I wonder if this should be an XFAIL, not UNSUPPORTED. The idea being that when the issue is fixed, the test will start x-passing, and not be forgotten about.
> > I think we should stick with `UNSUPPORTED` here because the original failure was due to some change specific to VS2022, and the minimum version to build LLVM is currently VS2019. This means that there might be some buildbots/developers/users building LLVM on VS2019 where this test would unexpectedly pass.
> In fact, current build bots still use VS2019 which makes the test succeed on Windows too. The issue with VS2022 was revealed later upon downstream builds with VS2022.
Ah, yes, sorry for the noise. It would be nice to have the ability to XFAIL for specific compiler versions, but I don't think that functionality exists in lit.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml:25
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content: 554889E5897DFC8B45FCC1E0015DC390554889E5897DF8837DF8050F8E0E0000008B45F883E8058945FCE9060000008B45F88945FC8B45FC5DC3
+  - Name:    .llvm_bb_addr_map
----------------
aidengrossman wrote:
> jhenderson wrote:
> > This Content is a bit of an opaque blob. Would it be possible and clearer to write this test using assembly instead?
> The content is definitely quite opaque. From what I can gather from the yaml2obj tests, it is possible to write these tests in assembly (without piping anything around), but all the content has to be in hex with assembly commentary beside it. The current precedent for tests regarding this feature is to use similar opaque blobs (although maybe this should be changed at some point). However, I'm not sure how much value adding assembly would bring. The assembly is essentially the same as the disassembled input that I'm checking later on plus a couple extra irrelevant annotations. For debugging the test case, I think having the expected output would be the most helpful and adding assembly for the input would not add a lot of value. I might be missing something obvious here though, so please let me know if I'm incorrect in one of my points or there's something I haven't thought of.
I was referring to making the whole input an assembly file and not using YAML at all. However, with the removal of the opaque blob, that no longer is relevant.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7096
+  };
+  Expected<llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *>> SecRelocMapOrErr = this->Obj.getSectionAndRelocations(IsMatch);
+  if(!SecRelocMapOrErr) {
----------------
Not clang-formatted.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7098
+  if(!SecRelocMapOrErr) {
+    handleAllErrors(SecRelocMapOrErr.takeError(), [&](StringError &SE) {
+      this->reportUniqueWarning(SE.getMessage());
----------------
I have a slight concern that this is slightly fragile - if the underlying code changes to emit other kinds of errors, this will fail here. It should probably handle the error base class, in some manner, so that we don't end up with a surprise crash in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841