Looks like GCC warns on everything but the last unused expression (the last one being the return statement) - only warning on that last one if it's ultimately unused by the statement expression function call, as it were (& is annotated warn_unused_result). Basically, it models it like a function in a way that Clang perhaps does not?

Looks good - thanks!

In D77611#1967488, @nickdesaulniers wrote:

Note your test case is related to -Wunused-value, mine is -Wunused-result (via the use of __attribute__((warn_result_unused)) which we should always warn about.

There are a few places where smaller-scoped BitcodeWriters are used where a SmallVector may be suitable, but I'm OK sacrificing those for the broader goal. If someone finds these to be performance sensitive, we can revisit/try to find some way to support both goals.

Thanks for this!

(side note: Clang doesn't strive to emulate GCC's warnings bug-for-bug/exactly - certainly about tradeoffs in terms of true and false positives to decide what's appropriate for a clang warning)

In D77611#1965824, @nickdesaulniers wrote:

ah, using this to build a Linux kernel triggers tons of warnings. So the behavior of GCC is even more subtle than captured in the test cases here.

Looks good - thanks!

In rGfabe52a7412d850bf06e006db1d53de0500a1853#914199, @RKSimon wrote:

That one is a bit of blur (a lots happened since then.....) but I think it was one of the more obscure buildbots? Maybe one of the clang-ppc64 bots running a funny gcc version?

Agree in theory - but given the behavior has been different for quite a while, I think it might be worth a bit of an inventory of uses of BitVector and of BitVector's op== existing behavior - and maybe a rough explanation for why it's not problematic to change it in this way.

@aprantl Would probably prefer one of you folks more familiar with dsymutil/DWARFLinker to do final review on this

Looks good - some of the suggestions I made don't hold or hold less with one of your other changes applied (truncated data extractors) - but might still be worth considering even combined with that.

In D77308#1963790, @labath wrote:

In D77308#1958553, @aprantl wrote:

I guess this might be unit-testable by supplying too-short DataExtractors?

That is what the code DWARFFormValueTest is doing. Maybe it's somewhat over-engineered (I would have just written a sequence of EXPECTS, instead of instantiating a separate test for each form), but I tried to follow the style of the other tests in that file.

@dblaikie: Is that what you had in mind, or were you expecting to see some higher-level tests?

Some nice cleanup indeed - thanks! Please wait for @jhenderson to also approve, since he's been doing more work in this area. (never sure whether to approve or not in this case - not sure if the official "approval" might then remove it from other people's review queue, etc)

Looks good - thanks! (I guess at some point we might want a DWARFDataExtractor that can be offset as well - for reading sections in DWP files (essentially create a view of the file based on the index, restricting a CU to only being able to see (& resolve references relative to) the regions described by the index))

Looks good - thanks!

(copied from llvm-commits@)

Looks good - thanks!

Ping

Looks good - thanks!

Sure - looks good

Seems good - thanks!

Looks good - thanks!

Looks good to me - please wait for @ikudrin to circle back on this and approve it as well.

In D77421#1960852, @inglorion wrote:

@dblaikie: My understanding is that LLVM reserves the right to assume that distinct globals never alias and optimize based on that (e.g. as mentioned under https://releases.llvm.org/10.0.0/docs/AliasAnalysis.html#the-basicaa-pass), and that declaring globals as zero-sized is the accepted way to avoid that (e.g. as mentioned in https://bugs.llvm.org/show_bug.cgi?id=31675#c5).

So it's not so much improperly applied TBAA as it is an assumption that LLVM allows itself to make regardless of type, with a special exception for zero-sized types.

Rather than replying point-by-point, since I think the conversation has got a bit jumbled up - and maybe it's all best left to a separate thread than in this review... but:

Looks good - thanks!

Naive question, but I thought LLVM IR didn't have type based alias analysis based on LLVM IR pointer types (given the intention to remove pointee types - and any cases of pointee types being significant to optimizations are bugs, so far as I know). Is that's what's going on here? Because perhaps the right fix is to fix the optimization that's making assumptions based on pointee types instead of this change?

(***) fdebug-types-section is not supported by dsymutil/DWARFLinker.
Thus presented results do not show real data.
But these numbers could be taken as an estimation for future results
when DWARF5/-fdebug-types-section would be supported by dsymutil/DWARFLinker.

In D74169#1960503, @clayborg wrote:

Nice! Another thing I have been seeing a lot of is when functions are coalesced and many .o files have the same function that gets linked to the same location. We end up with many duplicate DWARF DIEs in the final executable since the linker will just link all of the DWARF for each function over and over (.debug_info and .debug_line content). I found this when making GSYM files from the DWARF and noticing that I had hundreds of duplicate entries that matched exactly. This would be a great follow up patch for --gc-debuginfo

In D74169#1960444, @avl wrote:

Not so much reason to support type units ("-fdebug-types-section/type units/.debug_types section/multiple .debug_info sections" all essentially describe the same thing). Since type units add object size overhead to reduced linked size in the face of a DWARF-agnostic/ignorant linker. If you have a DWARF aware linker, you'd probably avoid using type units so you could have smaller object files too.

It seems to me type units are useful for the current scheme. They increase the size of type reference(8 bytes vs 4 bytes) and introduce type unit header, but they avoid duplication. All references inside the object file point to the single type description - thus, object files have only one type's description instead of multiple copies. The effect of de-duplication is higher than the introduced increase.

DWARF aware linker does not reduce the size of object files. It reduces the size of binary, whereas type units allow decreasing object file.

In D74169#1960135, @avl wrote:

This version is a full patch implementing "remove obsolete debug info from lld" functionality.
As it was requested, It is presented as a full patch, which could be applied to the current code
base and evaluated. Some parts of that patch are already presented for review - D77169, D76085.
I am going to add tests for this patch and to address comments. Performance results would be presented in a separate message.

Limitations: it does not support DWARF5, modules, -fdebug-types-section, type units, .debug_types, multiple .debug_info sections.

Couple of small nits, but I'll leave most of the review to someoen else here - I /think/ it's beyond my context/experience (but if necessary, poke me and I can look more/ask more questions/etc)

I think it's OK - best I can tell at this point, given all the review iterations/comments/etc. *fingers crossed*

Sounds good :)

Looks good - thanks!

Sounds good - thanks!

This change adds a bunch more error handling, if I understand correctly (previously extractValue would've silently succeeded (it could only return true) where now it will fail) - I'd expect some negative test cases demonstrating the new failure modes/error cases being handled? Is there a reason they're not tested?

In D75929#1954443, @labath wrote:

Thanks for doing this. This looks fine to me. @dblaikie, @jhenderson, do you have any additional comments?

This wasn't well committed, unfortunately.

Should this have any tests (unit or otherwise)?

In D77154#1953480, @hliao wrote:

In D77154#1953423, @MaskRay wrote:

This seems fine. I have -DCMAKE_EXE_LINKER_FLAGS=-Wl,--gdb-index -DCMAKE_SHARED_LINKER_FLAGS=-Wl,--gdb-index -DLLVM_USE_SPLIT_DWARF=On in my long cmake command line. I am just wondering whether this new option will be discoverable.

Some people don't use -DLLVM_USE_LINKER=lld, but use -DLLVM_ENABLE_LLD=On.

As I mentioned in https://reviews.llvm.org/D58628, if your system's default linker doesn't recognize --gdb-index, adding option into CMAKE_EXE_LINKER_FLAGS fails the make detection stage, where the default linker is used.

Looks good - thanks!

In D74589#1947218, @sbc100 wrote:

Will do

Could you include a few of the details from the history there to explain it in the commit message?

The change description goes into quite a bit of detail already no? Or am I missing something?

Sounds good - please commit the StringPool fix separately & I'd personally probably remove the wasm test, but I can see the merit in including it too, so up to you there.

In D75684#1946846, @mehdi_amini wrote:

In D75684#1944248, @dblaikie wrote:

I received an email on the approval, can you clarify?

Ah, guess it might send it specifically to the author only, then?

I am only subscriber on this revision, just like the mailing list actually I think.

But it doesn't seem to make it to the mailing list: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200302/thread.html#751858

Our comments (mine and yours) above haven't ended up on the list either apparently,

In D72092#1945523, @MaskRay wrote:

In D72092#1945410, @dblaikie wrote:

@MaskRay I know it's a bit quirky, but Phabricator doesn't send email to the list on status changes (like approvals) that contain no user-authored text, which makes it look like patches are being committed without approval (if you're just reading the mailing list). If you could write a little something ("Thanks", "Looks good", etc) whenever approving a patch, it'd help make sure the mailing list accurately reflects the state of patches - thanks!

@dblaikie I actually read a similar comment from you to another person. Thanks for the reminder! To prevent me from forgetting, I created a Tampermonkey script:

const select = document.querySelector('.aphront-form-input > select')
if (select)
  select.onchange = function() {
    const accept = select.querySelector('option[value=accept]')
    if (accept && accept.disabled) {
      const comment = document.querySelector('.remarkup-assist-textarea')
      if (comment && comment.value.length == 0) {
        const words = ['LGTM.', 'Thanks!', 'Looks great!']
        comment.value = words[~~(Math.random()*words.length)]+' '
        comment.focus()
      }
    }
  }

@MaskRay I know it's a bit quirky, but Phabricator doesn't send email to the list on status changes (like approvals) that contain no user-authored text, which makes it look like patches are being committed without approval (if you're just reading the mailing list). If you could write a little something ("Thanks", "Looks good", etc) whenever approving a patch, it'd help make sure the mailing list accurately reflects the state of patches - thanks!

Super handy - thanks! (let me know if you need me to commit this for you, otherwise go ahead and commit it yourself)

In D74781#1944970, @sbc100 wrote:

Regarding the .debug_loc section. I did a quick experiment it looks like, even before this patch we get the expected output:

test.o:	file format WASM

.debug_loc contents:
0x00000000: 
            [0x00000005, 0x00000009): DW_OP_consts +21, DW_OP_stack_value

0x0000001d: 
            [0x0000000a, 0x0000000e): DW_OP_consts +7, DW_OP_stack_value

It looks like reason this works is because there is only one relocation per function here. i.e. there are only two relocations in the above object code. I appears the individual ranges do not themselves contain any relocations.

In D74781#1944828, @sbc100 wrote:

Regardless of any longer term plans for a new type of DWARF linking it seems that this change is strictly an improvement since (a) it matches the behavior of the existing ELF linker and (b) existing tools already handle this 0 ... N range.

Any objections to landing this now?

In D74781#1940564, @paolosev wrote:

In D74781#1939787, @dblaikie wrote:

In D74781#1938336, @paolosev wrote:

@sbc100 Adding a test.

@dblaikie Strangely, I can't find a way to generate a .debug_loc section for Wasm.

Maybe it's not applicable to wasm, I don't know much about it - but if it optimizes variables into registers (or out of them), then it shuold be producible.

Here's a simple test case I use for debug_loc in C++ on a silicon target:

void f1();
int main() {
  int i = 3;
  f1();
  i = 7;
  f1();
}

Compiled with optimizations enabled, the storage for 'i' will be eliminated and a DWARF location list will be used to describe 'i' as having the value 3 over the first call instruction, and the value 7 over the second call instruction.

Yes, I managed to get a .debug_loc section also for Wasm with similar code, and debug_loc does not seem to present the same problems with GC'd functions.

naming of autonamed values in -print-after=si-wqm output.

@lhames just for some examples of debugging and orc jit

Probably best to test this functionality with a unit test for StringSet - but also please check if there's a reason StringSet might not support empty strings in its implementation in some way? (tombstone/other marker objects might be using "length 0" as a marker of some kind?)

In D75684#1942522, @mehdi_amini wrote:

In D75684#1942146, @dblaikie wrote:

Just a quick note - PHabricator has a bug where it doesn't send email when status changes are made without text. @ftynse please include some text when approving patches to ensure that approval is reflected on the mailing list as well.

I received an email on the approval, can you clarify?

@gribozavr2 - hey, an annoying quirk of Phabricator is that it doesn't send mail to the mailing list on state changes with no text, which means reviews like these look like tehy were committed without approval (on the mailing list there's no record of the approval). In the future, can you add some comment to your approval ("Looks good", "thanks", etc) to ensure mail is sent to the mailing list to document that approval there?

Just a quick note - PHabricator has a bug where it doesn't send email when status changes are made without text. @thakis please include some text when approving patches to ensure that approval is reflected on the mailing list as well.

Just a quick note - PHabricator has a bug where it doesn't send email when status changes are made without text. @ftynse please include some text when approving patches to ensure that approval is reflected on the mailing list as well.

Pinging @lhames

For some reason the order in which we call getNegatedExpression for the involved operands, after a call to isCheaperToUseNegatedFPOps, seem to matter.

ah, good to see there's already some verbiage here that covers the general idea (which I'd clearly forgotten about, might've been good framing for the discussion earlier, so good to know it's here) - specifically saying "here's a better thing" but "use it" is left as an implication - the new paragraph makes that more explicit, which is good.

In D74781#1938336, @paolosev wrote:

@sbc100 Adding a test.

@dblaikie Strangely, I can't find a way to generate a .debug_loc section for Wasm.

Seems reasonable to me - thanks!

In D6379#1936534, @ostannard wrote:

It looks like the issues this is warning about isn't the .debug_line section, but the DW_AT_low/high_pc attributes of the compilation unit DIE. With DWARF3 we can emit a DW_AT_ranges attribute instead, but that didn't exist in DWARF2.

Since this range should only cover PC values corresponding to this compilation unit, maybe we could also limit this warning to sections with SHF_EXECINSTR?