[Lldb-commits] [PATCH] D52375: [WIP] Support multiple compile units per OSO entry in SymbolFileDWARFDebugMap

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added inline comments.


================
Comment at: packages/Python/lldbsuite/test/macosx/debug_map/Makefile:13
+# Set inputs empty, if not passed from the Python script.
+CFLAGS ?=
+LDFLAGS ?=
----------------
aprantl wrote:
> This is a noop. You can remove it without affecting anything.
It was meant to indicate that the python script can override these parameters, but you are right. It's probably more confusing than helpful. Above comment should be enough.


================
Comment at: packages/Python/lldbsuite/test/macosx/debug_map/Makefile:30
+main.o: main.c
+	$(CC) -c -g $(CFLAGS) $(CFLAGS_MAIN) -o main.o $(SRCDIR)/main.c
+
----------------
aprantl wrote:
> `  $(CC) -c $(CFLAGS) $(CFLAGS_MAIN) -o $@ $^`
> 
> -g should already be part of CFLAGS, no?
> -g should already be part of CFLAGS, no?
Yes, right.

> `  $(CC) -c $(CFLAGS) $(CFLAGS_MAIN) -o $@ $^`
Actually, I'd prefer my version with `-o main.o $(SRCDIR)/main.c`. It's more verbose, but clearly states what happens here. `$@ $^` is hard-to-read Makefile magic (to me).


================
Comment at: packages/Python/lldbsuite/test/macosx/debug_map/Makefile:42
+a.out: main.o cu1.o cu2.o cu3.o
+	$(LD) main.o cu1.o cu2.o cu3.o -L. $(LDFLAGS) -o a.out
----------------
aprantl wrote:
> ` $(LD) $^ -L. $(LDFLAGS) -o a.out`
> 
> or even shorter, remove the entire line and just declare the dependencies, because the default rule from Makefile.rules will kick in then.
Not sure why, but removing the line causes a lot of compile errors, like
`clang-8: error: no input files` and 
`Undefined symbols for architecture x86_64: "_main", ...`

I guess I had to declare my `OBJECTS` then, but it's cleared in Makefile.rules. In the end, it's one line more, but it's not unreasonable right? Maybe I could just keep it.


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp:400
+          // separately. It may be turned into a m_oso_entry_info.
+          m_compile_unit_infos.push_back(oso_info);
+          CompileUnitInfo &cu_info = m_compile_unit_infos.back();
----------------
It turned out, that adding the actual number of compile units to this vector, has a number of scary side effects, which fail tests on the other side of LLDB. I had a closer look at one issue (parsing variables in stack frames for expression evaluation). I could fix this one, but it needs extra code changes. I can imagine these to introduce new silent failures, etc.

I will try again to get the patch more conservative, but it looks more and more like this needs a comprehensive refactoring.


https://reviews.llvm.org/D52375