Is it possible you need to look only at the immediately preceding field, and not iterate? For example,

struct zero_bitfield {
  char a : 8;
  char : 0;
  char b : 8;
  char c : 8;
};

If processing b sees the zero-length bitfield and does the needful, then when processing c it's sufficient to see that b is a preceding bitfield and know that the needful has been done.

I've only skimmed the discussion, but FTR, Sony licensees are also in a single-compiler environment, which is enforced in a couple of ways. In particular we have a downstream patch to insert the compiler version into the bitcode (modeled on similar code for Darwin IIRC) and the linker will complain if it sees a different version, before starting the LTO machinery.

LGTM too

+debug-info

In D143513#4178528, @benmxwl-arm wrote:

(I'm still using // in the test since llvm-mc with the arm-none-eabi triple does not accept
\# or \#\# as a comment).

I think the GC behavior with explicit section names is currently a little peculiar. For functions without a section name, -ffunction-sections allows GC to happen at the individual function level. With a section name, GC would happen at the level of all-or-nothing per input file, regardless of -ffunction-sections. That just seems unexpected and inconsistent to me.
But it is the current behavior, and given the objections it's not worth pursuing this.

Defining UnknownLocations as extern in LexicalScopes.cpp gives a link-time error despite removing the "static" keyword from its definition in DwarfDebug.cpp.

You don't want to use DW_AT_description for this?

See D145173 for a different tactic to solve this.

I've looked at this but I'd like someone more in tune with MSVC behavior to review as well.

It seems strange to define the enum in a header for a module that uses it only once, and externalize it in a module that uses it several times.

Tidy up the test and LGTM.

Looks like I made this reach the 50% mark, so I think you're good to go.

Sony has two reps for much the same reason. LGTM.

Sorry for letting this sit so long! Code looks okay, still needs a test. For something like this you probably need an assembler source with a hand-made .debug_line section.

That's correct, I do see <stdatomic.h> in our SDK.

I looked at this test only because it caused a merge conflict downstream. While it does work, it will not catch some kinds of mistakes; by being less specific in the "not" checks, it will catch more potential problems.

Discussion on the GCC bug has persuaded me this is not a good idea. I'll solve my user's problem a different way.
@MaskRay you can close the GCC bug, it looks like I can't do it myself.

+debug-info tag.

I'm not sure I understand the linker's mechanics here. Let me say some things and you can describe my misunderstanding.

• If the linker was going to discard all of section foo (in the current scheme), that means it had no reason to retain either f() or g(). In the new scheme, the net result would be the same, both f() and g() are dead and would be discarded. So, no behavior change.
• If the linker wanted to retain f(), but had no reason to retain g(), then in the current scheme it would retain all of section foo. In the new scheme it would retain f() but discard g(). This is the desired behavior change.

LGTM. A revert isn't cheap when it reverts a test to a broken state.

This works in my simple test cases, but I'm not 100% sure it's (a) the best place to do this, or (b) the only place that needs to do this.
Really we should guarantee that comdat any wins over comdat nodeduplicate in all cases.

LGTM

The comment suggests that it wants a (default) target that has particular characteristics, so updating REQUIRES to match that intent seems like a better approach.

LGTM, but you might give @arsenm another day before committing.

I don't know if this can happen, but what about an LTO scenario with different values for the module flag? I seem to recall module flags needing to provide a merge policy? I might be remembering something else though.
If the result is "fail the compilation" that seems fine.

Examples added to the commit log.
Entry made in release notes.
I will keep an eye out for bot fail-mail related to this change.

In D141007#4059797, @ldionne wrote:

One thing to keep in mind here is that folks who rebase patches on top of this patch may get bitten by this issue for a couple weeks, so it may be worth grepping for suspicious patches from time to time, or turning this into an explicit error instead (just for a few weeks).

In D141007#4059777, @yln wrote:

I had just one question: what kind of testing did we do / how did we discover all uses of the triple substring matches?

@tra can I get your okay on this?

Ping. Add @yln

Basically you're homing a pointer to the parameter, rather than the parameter itself, which makes sense in this context.

Add "This fails today ..." notes to the tests where I added XFAIL: *

I should expand on my previous comment. Extending the lifetime of these variables by saving the pointers into local variables, that's fine; just make sure it's not conditional on -g. The only difference between -g and not -g should be metadata and dbg.* instructions.

This causes codegen to be different depending on whether debug-info is generated. Please don't do that.

One minor comment. I don't feel comfortable enough with metadata to actually approve this, however.

Would [target=xxx] be true when the default target matches the one specified by target=xxx, or when the compiler does have enabled back-end for that target?

target=xxx would be true when the default triple matches. The check for having the backend enabled is still xxx-registered-target.

Ping

Ping. +tra based on git-log

So, the real problem is, once somebody managed to herd the cats all into the same direction (or close enough), the consensus was "no mass conversion to the new style." Which naturally left everything inconsistent.

Please generate diffs with full context (e.g., -U999999 in the diff/show command). It's really helpful to be able to look above and below the modified lines to understand the context of the patch.

ping

Example updated.

Please use clang-format-patch to correct some style issues.

Worth trying on MSVC, which is the other more likely place to fail.

In D140123#4000196, @arsenm wrote:

I feel this is too specific and doesn't belong in the datalayout. I think a general maximum global alignment would make a bit more sense? I know very little about object formats