Page MenuHomePhabricator

[DWARF] Fix the reserved values for unit length in DWARFDebugLine.
ClosedPublic

Authored by ikudrin on Jul 12 2019, 2:30 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ikudrin created this revision.Jul 12 2019, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 2:30 AM
Herald added a subscriber: aprantl. · View Herald Transcript

This magic number appears in many places; it should have a symbolic name in llvm/include/llvm/BinaryFormat/Dwarf.h, in the LLVMConstants enumeration.

Right, but maybe that worths another patch?

Right, but maybe that worths another patch?

A quick grep suggests there are not very many more places that you would have to change. You can limit this patch to DebugInfo/DWARF if you like; I expect there are more places in CodeGen/AsmPrinter, which could be done separately.

I mean, the purposes for the patches are different. This one fixes an existing flaw while your suggestion improves code quality. It is usually better not to intermix such different aims in one patch, no?

I mean, the purposes for the patches are different. This one fixes an existing flaw while your suggestion improves code quality. It is usually better not to intermix such different aims in one patch, no?

There is no hard-and-fast rule about this; the project does prefer incremental changes, but the determination of what "size" increment to perform is not strictly defined. Personally I would do it all at once, with the stated purpose being to clean up the use of magic numbers (some of which happen to be incorrect). If you want to work in smaller increments, that's okay too.

I see that there are no tests verifying that DWARFDebugLine handles the invalid length correctly (assuming you ran the tests and none failed), so please add one.

I see that there are no tests verifying that DWARFDebugLine handles the invalid length correctly (assuming you ran the tests and none failed), so please add one.

There were three tests in unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp which failed, so they are also updated in this patch. Or do you mean something different?

probinson accepted this revision.Jul 15 2019, 8:15 AM

If you found the unittests cases because they failed, that's good enough for me.
LGTM

This revision is now accepted and ready to land.Jul 15 2019, 8:15 AM

Thanks! I'll prepare a patch to symbolize these magic numbers a bit later.

This revision was automatically updated to reflect the committed changes.