Page MenuHomePhabricator

[lldb] Fix DWARF-5 DW_FORM_implicit_const (used by GCC)
ClosedPublic

Authored by jankratochvil on Mar 8 2021, 9:50 AM.

Diff Detail

Event Timeline

jankratochvil created this revision.Mar 8 2021, 9:50 AM
jankratochvil requested review of this revision.Mar 8 2021, 9:50 AM
jankratochvil added inline comments.Mar 8 2021, 9:53 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.cpp
30

This const_cast is not nice. For its removal one can do either of:

jankratochvil planned changes to this revision.Mar 8 2021, 10:01 AM

Could you simplify the test case? It sounds like all you need is a single global variable (int foo = 47;), without any functions or that stuff. I guess we should also delete test/Shell/Breakpoint/implicit_const_form_support.test, as it's not very useful (the numbers it checks come from the line table, not from the implicit constants). Other than that, this seems fine.

lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.cpp
30

this is not the first const_cast of DWARFUnits, so, while definitely not ideal, I'm not particularly worried about it.

lldb/test/Shell/SymbolFile/DWARF/dwarf5-implicit-const.s
3

I hope these are not necessary, since you don't run the code here.

jankratochvil marked 2 inline comments as done.Mar 9 2021, 12:54 AM

Could you simplify the test case?

done

I guess we should also delete test/Shell/Breakpoint/implicit_const_form_support.test, as it's not very useful (the numbers it checks come from the line table, not from the implicit constants).

I have not deleted it myself; I find non-racy simple tests to be always good as they may catch some completely unrelated regression in the future.

lldb/test/Shell/SymbolFile/DWARF/dwarf5-implicit-const.s
3

OK, removed:

-# UNSUPPORTED: system-darwin, system-windows
-# REQUIRES: x86

We will see after all the bots run again.

labath accepted this revision.Mar 9 2021, 1:10 AM

I guess we should also delete test/Shell/Breakpoint/implicit_const_form_support.test, as it's not very useful (the numbers it checks come from the line table, not from the implicit constants).

I have not deleted it myself; I find non-racy simple tests to be always good as they may catch some completely unrelated regression in the future.

Fine, I'll do it afterwards. :) If it was at least testing what it claims it's testing, I might keep it. However, judging by the need to re-fix implicit const support, it's not doing a good job at even that.

This revision is now accepted and ready to land.Mar 9 2021, 1:10 AM
labath added inline comments.Mar 9 2021, 1:14 AM
lldb/test/Shell/SymbolFile/DWARF/dwarf5-implicit-const.s
3

Sorry, I only meant the system-*** thingies. The REQUIRES: x86 thingy was correct, as it will cause the test to be skipped if someone completely disables x86 support (via LLVM_TARGETS_TO_BUILD).

That said, no bot actually builds lldb in that way, so I'm sure we have a lot of tests which are missing this clause.

jankratochvil marked 2 inline comments as done.Mar 9 2021, 1:22 AM
This revision was landed with ongoing or failed builds.Mar 9 2021, 1:24 AM
This revision was automatically updated to reflect the committed changes.