Thanks for the review!

Ping.

In D71834#1820359, @aprantl wrote:

But removing the default values is also seems to be out of the scope of the patch.

Okay, but could you please prepare a follow-up patch that does this?

In D71546#1797665, @dblaikie wrote:

Was this fixing an existing buildbot/test failure? (if not, it should probably have a test? If it did fix an existing failure - could you mention what that failure was?)

• Used curly brackets for the while loop in dumping .debug_aranges in DWARFContext::dump().
• Removed else branch after break.

Another 1U is changed to 1ULL. Thanks, @aprantl!

Ping.

LGTM. Sorry for the delay.

In D71932#1797512, @dblaikie wrote:

Could these tests all be in one run? (ie: even though the length is "invalid" in terms of parsing the contents of the section - but valid enough to jump to the next contribution and parse that - except, I guess, at an invalid 64 bit length perhaps, because that might be too long to be bothered testing - so perhaps that could be the last test case in the file)

• Made the diagnostic a bit more distinct.
• Moved the tests to a gtest-based suite.

• Hardcode paddings.
• Move some tests to the gtest-based suite.

• Update wordings.
• Replace most of the lit tests to check error cases with gtest-based ones.
• Do not silently absorb the error messages.
• Update existing tests and add new ones to reflect the changes.

• Use hardcoded padding.

• Add a macro and an explanation comment for the padding wizardry in the test. Hope it is more clear now.

Thanks for the suggestions, @dblaikie! I've added D71931 and D71932 with corresponding patches.

• Updated as the parent patch is changed;
• Added more tests.

• "!x" -> "x == 0";
• Improved error messages wordings;
• Added more tests;
• Fixed checking the value of the Length field.

Actually, I've discovered that we parse range list table headers for every unit, including type units, even if they do not reference them. Thus, this patch creates more problems than solves; it should be postponed until we make extracting range list tables lazy and do it only for compilation units which actually reference the tables.

• -1U -> -1ULL. Thanks, @grimar!

• Rebase to the tip.
• Add reporting the offset of a compilation unit which format is taken as a reference.

• Add a test for a CU with missing fields which was considered valid before the patch.

It looks like lld/test/ELF/undef.s comes here by accident, right?

Personally, I would prefer to see the file name and path to be changed as little as possible because that would help to recognize the files better. We cannot use remove_dots() on POSIX OSes to simplify paths, because it may return an invalid path; thus we have to use getRealPath(). If I understand it right, there is no similar problem with the file name itself.

I'd like to note that this is not a purely theoretical issue. It can be observed, for example, in Symbolizer::ModuleNameOwner on a 64-bit platform with 4K memory pages. The default constructor for storage_ allocates one memory page, which is immediately replaced by a new allocation in the call to reserve() for 1000 elements, which requires 8 * 1000 = 8000 bytes, or two pages.

Sorry to intervene, but what about the patch itself? Is it OK to land with the latest changes or should I improve it somehow?

• Rebase on the current tip.

• Remove redundant log files. Thanks, @MaskRay!

• Use --implicit-check-not=error.

In D67097#1658364, @dblaikie wrote:

This update didn't seem to touch a test case - is this untested?

• Print the adjusted offset which points to the start of the header.

Ping.

In D67097#1656268, @dblaikie wrote:

I'm not sure if using "OffsetIn" in the diagnostic is the right thing to do.

• Added a check for reserved values of the length field.