Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Differential D82173 Diff 272411 llvm/lib/ObjectYAML/DWARFVisitor.cpp

Changeset View

Standalone View

View Options

llvm/lib/ObjectYAML/DWARFVisitor.cpp

//===--- DWARFVisitor.cpp ---------------------------------------*- C++ -*-===// //===--- DWARFVisitor.cpp ---------------------------------------*- C++ -*-===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "DWARFVisitor.h" #include "DWARFVisitor.h"
#include "llvm/ObjectYAML/DWARFYAML.h" #include "llvm/ObjectYAML/DWARFYAML.h"
#include "llvm/Support/ErrorHandling.h"
using namespace llvm; using namespace llvm;
template <typename T> template <typename T>
void DWARFYAML::VisitorImpl<T>::onVariableSizeValue(uint64_t U, unsigned Size) { void DWARFYAML::VisitorImpl<T>::onVariableSizeValue(uint64_t U, unsigned Size) {
switch (Size) { switch (Size) {
case 8: case 8:
onValue((uint64_t)U); onValue((uint64_t)U);
Show All 22 Linesstatic unsigned getRefSize(const DWARFYAML::Unit &Unit) {
return getOffsetSize(Unit); return getOffsetSize(Unit);
} }
template <typename T> void DWARFYAML::VisitorImpl<T>::traverseDebugInfo() { template <typename T> void DWARFYAML::VisitorImpl<T>::traverseDebugInfo() {
for (auto &Unit : DebugInfo.CompileUnits) { for (auto &Unit : DebugInfo.CompileUnits) {
onStartCompileUnit(Unit); onStartCompileUnit(Unit);
if (Unit.Entries.empty()) if (Unit.Entries.empty())
continue; continue;
auto FirstAbbrevCode = Unit.Entries[0].AbbrCode;
for (auto &Entry : Unit.Entries) { for (auto &Entry : Unit.Entries) {
onStartDIE(Unit, Entry); onStartDIE(Unit, Entry);
if (Entry.AbbrCode == 0u) uint32_t AbbrCode = Entry.AbbrCode;
if (AbbrCode == 0 || Entry.Values.empty())
continue; continue;
auto &Abbrev = DebugInfo.AbbrevDecls[Entry.AbbrCode - FirstAbbrevCode];
if (AbbrCode > DebugInfo.AbbrevDecls.size())
// TODO: Handle and test this error.
grimarUnsubmitted
Not Done
ReplyInline Actions

Doesn't seem this assert is helpful. AbbrevDecls is std::vector<Abbrev>,
Most (or all) of STL implementations check the out of bounds access internally I think.

Probably you should add a test case and do the following for now:

if (AbbrCode > DebugInfo.AbbrevDecls.size())
  report_fatal_error(...)
grimar: Doesn't seem this assert is helpful. `AbbrevDecls` is `std::vector<Abbrev>`, Most (or all) of…
HiguoxingAuthorUnsubmitted
Done
ReplyInline Actions

Sorry, I haven't tested a report_fatal_error() before. What will be checked? The dumped stack trace? or something else? Can I leave a "TODO" here and let this function return Error in a follow-up patch?

Higuoxing: Sorry, I haven't tested a `report_fatal_error()` before. What will be checked? The dumped stack…
jhendersonUnsubmitted
Not Done
ReplyInline Actions

Typically, you wouldn't test a report_fatal_error as it shouldn't be possible to reach that bit of code. It's like an assert, but works without asserts being enabled too.

jhenderson: Typically, you wouldn't test a `report_fatal_error` as it shouldn't be possible to reach that…
grimarUnsubmitted
Done
ReplyInline Actions

What will be checked? The dumped stack trace? or something else?

Just the fact of crash of the test case. In the follow-up you could fix the crash then.
It should be possible to check that crash with --crash option for not:

RUN: llvm-mc -filetype=obj -triple aarch64-windows %s -o - \
 RUN: | not --crash llvm-readobj --unwind - 2>&1 | FileCheck %s

I do not mind leaving the test case for a follow-up. It was just important not to use an assert here.
report_fatal_error calls abort().

grimar: > What will be checked? The dumped stack trace? or something else? Just the fact of crash of…
HiguoxingAuthorUnsubmitted
Done
ReplyInline Actions

Thanks! I added one more test case for it :-)

Higuoxing: Thanks! I added one more test case for it :-)
jhendersonUnsubmitted
Done
ReplyInline Actions

I'd change "Report" to "Handle", since the error is now being reported (although not in the best way).

jhenderson: I'd change "Report" to "Handle", since the error is now being reported (although not in the…
report_fatal_error(
grimarUnsubmitted
Done
ReplyInline Actions

Since you are touching this line, could you convert the auto to an actual type?

grimar: Since you are touching this line, could you convert the `auto` to an actual type?
jhendersonUnsubmitted
Done
ReplyInline Actions

Should this message be "... less than or equal to ..."?

jhenderson: Should this message be "... less than or equal to ..."?
"abbrev code must be less than or equal to the number of "
"entries in abbreviation table");
const DWARFYAML::Abbrev &Abbrev = DebugInfo.AbbrevDecls[AbbrCode - 1];
auto FormVal = Entry.Values.begin(); auto FormVal = Entry.Values.begin();
auto AbbrForm = Abbrev.Attributes.begin(); auto AbbrForm = Abbrev.Attributes.begin();
for (; for (;
FormVal != Entry.Values.end() && AbbrForm != Abbrev.Attributes.end(); FormVal != Entry.Values.end() && AbbrForm != Abbrev.Attributes.end();
++FormVal, ++AbbrForm) { ++FormVal, ++AbbrForm) {
onForm(*AbbrForm, *FormVal); onForm(*AbbrForm, *FormVal);
dwarf::Form Form = AbbrForm->Form; dwarf::Form Form = AbbrForm->Form;
bool Indirect; bool Indirect;
▲ Show 20 Lines • Show All 114 Lines • Show Last 20 Lines