[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGaba5b91b699c: Re-land [CodeView] Add full repro to LF_BUILDINFO record (authored by aganea). Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. @dexonsmith The issue with `-grecord-command-line`/`-frecord-command-line` is that it isn't producing a self-standing command-line. For example: > clang-cl /c main.cpp /clang:-grecord-command-line /Z7 would record: `d:\\git\\llvm-project\\stage1\\bin\\clang-cl.exe --d

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea updated this revision to Diff 396804. aganea added a comment. This revision is now accepted and ready to land. Herald added subscribers: abrachet, mgorny. Rebase & simplify. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80833/new/ https://re

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a subscriber: dexonsmith. aganea added a comment. In D80833#3052551 , @dexonsmith wrote: > In D80833#2069411 , @aganea wrote: > >> In D80833#2069246 , @amccarth

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

dexonsmith added a comment. If you do go with "off-by-default", the natural driver option to use would be the existing `-grecord-command-line`. You could use the same plumbing through `-cc1` that it does. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

dexonsmith added a comment. In D80833#2069411 , @aganea wrote: > In D80833#2069246 , @amccarth wrote: > >> Does embedding full paths affect distributed builds or build reproducibility? > > It does affect reproducibi

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

santagada added a comment. In D80833#2069411 , @aganea wrote: > In D80833#2069246 , @amccarth wrote: > >> Does the full path to the tool end up only in the object file, or does this >> make it all the way to the fi

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. In D80833#2221866 , @aeubanks wrote: > This seems to be breaking determinism on Windows builds, see > https://crbug.com/1117026. Can this be reverted and fixed? Will do. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aeubanks added a comment. This seems to be breaking determinism on Windows builds, see https://crbug.com/1117026. Can this be reverted and fixed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80833/new/ https://reviews.llvm.org/D80833 ___

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

uweigand added a comment. In D80833#2123508 , @aganea wrote: > In D80833#2109172 , @uweigand wrote: > > > Hmm, with clang-cl it seems the driver is trying to use this: > > Target: s390x-pc-windows-msvc > > which o

Re: [PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

lla.com; > john.rea...@vmssoftware.com; ztur...@roblox.com; ...@gmail.com; > llvm-commits ; > stefan.reinal...@molecular-matters.com; Ulrich Weigand < > ulrich.weig...@de.ibm.com>; mlek...@skidmore.edu; Clang Commits < > cfe-commits@lists.llvm.org>; Han Shen > *Objet

RE: [PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

; Amy Huang ; dma...@mozilla.com; john.rea...@vmssoftware.com; ztur...@roblox.com; ...@gmail.com; llvm-commits ; stefan.reinal...@molecular-matters.com; Ulrich Weigand ; mlek...@skidmore.edu; Clang Commits ; Han Shen Objet : Re: [PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

Re: [PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

You'll probably want the assert as well: assert.h assertion failed at llvm-project/llvm/lib/MC/MCStreamer.cpp:134 in virtual void llvm::MCStreamer::emitIntValue(uint64_t, unsigned int): (isUIntN(8 * Size, Value) || isIntN(8 * Size, Value)) && "Invalid size" On Fri, Jul 10, 2020 at 4:03 PM Eric Ch

Re: [PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

I'm seeing tests fail with a crash. Can we revert the patch and attempted fixes and start working from there? Stacktrace for the curious :) @ 0x56420187cbbe llvm::MCStreamer::emitIntValue() @ 0x5641fec38899 llvm::MCStreamer::emitInt16() @ 0x5641ff73b337 llvm::CodeViewDe

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. In D80833#2109172 , @uweigand wrote: > Hmm, with clang-cl it seems the driver is trying to use this: > Target: s390x-pc-windows-msvc > which of course doesn't exist. Not sure what is supposed to be happening > here, but it seems

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

uweigand added a comment. Hmm, with clang-cl it seems the driver is trying to use this: Target: s390x-pc-windows-msvc which of course doesn't exist. Not sure what is supposed to be happening here, but it seems that it's falling back on s390x-linux since on s390x, Linux is currently the only sup

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. When using `clang-cl`, this code should set it to the right format, on any platform: https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/Driver.cpp#L1071 However maybe something else happens on big endian architectures? @uweigand Would you mind running the

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. In D80833#2108423 , @uweigand wrote: > > Line 4 here fails on s390x but not on other Unix flavors, see: > > http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33346/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adebug-in

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

uweigand added a comment. > Line 4 here fails on s390x but not on other Unix flavors, see: > http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33346/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adebug-info-codeview-buildinfo.c > > @thakis @uweigand Any ideas would could go wrong her

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea updated this revision to Diff 272556. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80833/new/ https://reviews.llvm.org/D80833 Files: clang/test/CodeGen/debug-info-codeview-buildinfo.c lld/COFF/PDB.cpp lld/test/COFF/Inputs/pdb_lines_1_r

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea marked 2 inline comments as done. aganea added a subscriber: uweigand. aganea added inline comments. Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:4 +// RUN: %clang_cl /c /Z7 /Fo%t.obj -fdebug-compilation-dir . -- %s +// RUN: llvm-pdbutil dump --types %t.o

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

thakis added a comment. I took a look. It consistently fails for me on mac and on Windows in a `git bash` shell, but it passes in cmd with unxutils. Maybe this needs something like eac56724fd955af0f8521557cacc57a83f371649 ?

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. Reverted in rG2ae0df5be7408a79524743762b6c74953f31b805 . I'll investigate the issue on my end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80833/new/ h

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. In D80833#2101690 , @thakis wrote: > Hm http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/16533 is > happy so it's not broken on all Windows machines. Guess I'll take a look on > what's going on. You can hold off on

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

thakis added a comment. Hm http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/16533 is happy so it's not broken on all Windows machines. Guess I'll take a look on what's going on. You can hold off on the revert for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. In D80833#2101636 , @thakis wrote: > This is still broken; bots have been red for a few hours now. Can we revert > and analyze async, to keep the bots green please? Yes I will revert, but I don't understand. All the tests pass he

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

thakis added a comment. This is still broken; bots have been red for a few hours now. Can we revert and analyze async, to keep the bots green please? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80833/new/ https://reviews.llvm.org/D80833 _

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

thakis added a comment. Failing on Windows too, actually: http://45.33.8.238/win/17931/step_10.txt Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80833/new/ https://reviews.llvm.org/D80833 ___ cfe-commi

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. In D80833#2101057 , @thakis wrote: > Looks like this breaks tests on mac: http://45.33.8.238/mac/15751/step_10.txt > > Please take a look. Checking now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

thakis added a comment. Looks like this breaks tests on mac: http://45.33.8.238/mac/15751/step_10.txt Please take a look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80833/new/ https://reviews.llvm.org/D80833 _

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. Thanks for taking the time @hans, @amccarth! I've split this into several smaller patches, to ease bisect if needs be: rGa45409d8855a1e4538990507ef25e9b51c090193 - [Clang] Move clang::Job::printArg to

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

This revision was automatically updated to reflect the committed changes. Closed by commit rG403f9537924b: [CodeView] Add full repro to LF_BUILDINFO record (authored by aganea). Changed prior to commit: https://reviews.llvm.org/D80833?vs=268906&id=271698#toc Repository: rG LLVM Github Monore

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

amccarth accepted this revision. amccarth added a comment. My only real concern was about the possible reproducibility impact of introducing absolute paths into the binaries. If @thakis and @hans are satisfied this is OK, then LGTM. Comment at: clang/test/CodeGen/debug-info-

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:3 +// RUN: llvm-pdbutil dump --types %t.obj | FileCheck %s +// RUN: %clang_cl /c /Z7 %s /Fo%t.obj -fdebug-comp

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea updated this revision to Diff 268906. aganea added a comment. Fix tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80833/new/ https://reviews.llvm.org/D80833 Files: clang/include/clang/Basic/CodeGenOptions.h clang/include/clang/Driv

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea updated this revision to Diff 268881. aganea marked 11 inline comments as done. aganea added a comment. Addressed comments (see inline). Ensured the generated .OBJ contains relative paths when passing `-fdebug-compilation-dir . -no-canonical-prefixes`. Repository: rG LLVM Github Monore

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea added a comment. In D80833#2073458 , @hans wrote: > a. The compiler path is only absolute because it was absolute when put into > argv[0] right? I don't see anything in the code that makes it absolute? I > imagine most build systems will invoke th

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans added a comment. > The only absolute paths that remain are: a. the compiler path > (`D:\llvm-project\buildninjaDebMSVC\bin\clang-cl.exe` in the yaml below) and > b. the `-internal-isystem` paths. However that is not an issue on our end, as > we're building with `-nostdinc` + nuget packages

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea marked 5 inline comments as done. aganea added a comment. In D80833#2071863 , @hans wrote: > In D80833#2071818 , @aganea wrote: > > > I didn't change the PWD/CWD stored in `LF_BUILDINFO`, it has already been

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea updated this revision to Diff 268359. aganea added a comment. Herald added subscribers: jfb, dexonsmith. - Using `sys::flattenWindowsCommandLine` instead of previous quoting function, as suggested by @hans - Added Clang tests for `-fdebug-compilation-dir .` - Added LLD support for making a

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

thakis added a comment. We don't store physical absolute paths in pdbs if you hold things right. Look for /pdbsourcepath: in http://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html . rnk's change seems to store the main TU's dir as cwd, which likely either takes the dir from

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans added a comment. In D80833#2071818 , @aganea wrote: > I didn't change the PWD/CWD stored in `LF_BUILDINFO`, it has already been > done by Reid a while ago: D53179 (it > already stores absolute paths) That's surpri

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea marked an inline comment as done. aganea added a comment. I didn't change the PWD/CWD stored in `LF_BUILDINFO`, it has already been done by Reid a while ago: D53179 (it already stores absolute paths) However absolute paths are stored by design, if we wan

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

amccarth added a comment. In D80833#2069874 , @thakis wrote: > The change description says something about PWD I believe that was a typo for CWD. > Please don't add code that stores absolute paths, but at the moment this > doesn't do that as far as I c

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

thakis added a comment. The change description says something about PWD (which makes me worried about build determinism, like amccarth), but I don't see anything about that in the diff. The diff as-is looks fine to me with hans's comment about duplication resolved in some form. Please don't ad

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea marked an inline comment as done. aganea added a comment. In D80833#2069246 , @amccarth wrote: > Does the full path to the tool end up only in the object file, or does this > make it all the way to the final executable or library? The `LF_BUILDIN

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

amccarth added a comment. Does the full path to the tool end up only in the object file, or does this make it all the way to the final executable or library? Does embedding full paths affect distributed builds or build reproducibility? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:320 + /// Executable and command-line used to create a given CompilerInvocation. + const char *BuildTool = nullptr; + ArrayRef CommandLineArgs; aganea wrote: > hans wrote: > > T

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea marked 2 inline comments as done. aganea added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:320 + /// Executable and command-line used to create a given CompilerInvocation. + const char *BuildTool = nullptr; + ArrayRef CommandLineArgs; ---

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea marked an inline comment as done. aganea added inline comments. Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:3 +// RUN: llvm-pdbutil dump --types %t.obj | FileCheck %s + +int main() { return 42; } Is there a way to launch an arbitrary pro

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:320 + /// Executable and command-line used to create a given CompilerInvocation. + const char *BuildTool = nullptr; + ArrayRef CommandLineArgs; The name BuildTool makes me think

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea updated this revision to Diff 267880. aganea marked an inline comment as done. aganea added a comment. As requested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80833/new/ https://reviews.llvm.org/D80833 Files: clang/include/clang/Basic

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

akhuang added inline comments. Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:886 + } // FIXME: Path to compiler and command line. PDB is intentionally blank unless // we implement /Zi type servers. Can the 'path to compiler and command line' p

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

aganea created this revision. aganea added reviewers: hans, amccarth, thakis, mstorsjo, akhuang. Herald added subscribers: llvm-commits, cfe-commits, hiraditya. Herald added projects: clang, LLVM. This patch adds some missing information to the `LF_BUILDINFO` which allows for rebuilding a .CPP wi