[PATCH] D86805: [lld-macho] create __TEXT,__unwind_info from __LD,__compact_unwind
Jez Ng via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 28 20:03:55 PDT 2020
int3 added a comment.
just a partial review for now... will come back and do a deeper reading next week after I understand a bit more about the CU format.
================
Comment at: lld/MachO/OutputSegment.cpp:54
+ for (const OutputSection *osec : sections) {
+ if (name.equals(osec->name))
+ return osec;
----------------
nit: StringRefs can be compared using `==`
================
Comment at: lld/MachO/UnwindInfo.cpp:1
+#include "UnwindInfo.h"
+#include "Config.h"
----------------
missing license header
================
Comment at: lld/MachO/UnwindInfo.cpp:58
+
+bool UnwindInfoSection::isHidden() const {
+ if (checkedCompactUnwindInputs)
----------------
I think this should be `isNeeded()`, not `isHidden()` -- "hidden" indicates that the section header is absent but that the section data itself should still be emitted
================
Comment at: lld/MachO/UnwindInfo.cpp:93
+ for (const Reloc &r : isec->relocs) {
+ if (auto *risec = r.target.dyn_cast<InputSection *>()) {
+ if (risec->getVA() == 0) {
----------------
nit: `targetIsec` would be clearer
================
Comment at: lld/MachO/UnwindInfo.cpp:139
+ if (a.second == b.second)
+ // When freqnecies match, secondarily sort on encoding
+ // to maintain parity with validate-unwind-info.py
----------------
frequencies
================
Comment at: lld/MachO/UnwindInfo.h:51-66
+ mutable std::map<uint32_t, size_t> commonEncodingIndexes;
+ mutable std::vector<std::pair<compact_unwind_encoding_t, size_t>>
+ commonEncodings;
+ mutable std::vector<uint32_t> personalities;
+ mutable std::vector<unwind_info_section_header_lsda_entry> lsdaEntries;
+ mutable std::vector<compact_unwind_entry> cuVector;
+ mutable std::vector<compact_unwind_entry *> cuPtrVector;
----------------
why the need to mark everything as mutable?
================
Comment at: lld/MachO/Writer.cpp:413-414
+ .Case(segment_names::linkEdit, 100)
+ // __LD,__compact_unwind isn't an output segment, so shove it past the end
+ .Case(segment_names::ld, 101)
.Default(0);
----------------
This seems a bit hacky. Do we really have to put the `__compact_unwind` sections in an OutputSegment? If we're not going to output them, could we just put all these InputSections in a vector contained in `UnwindInfoSection`?
Also, we may want to support emitting object files at some point via `-r`, in which case we'll actually want to emit the `__LD` segment at a valid position.
================
Comment at: lld/test/MachO/tools/validate-unwind-info.py:89
+
+ return sys.exit()
+
----------------
this seems unnecessary given that we'll be exiting anyway when this function returns
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86805/new/
https://reviews.llvm.org/D86805
More information about the llvm-commits
mailing list