Commiting Changes to llvm-project

AryanGodara · March 13, 2023, 8:07pm

I have made local changes to begin fixing this issue :-

"lifetime extension of temporary created by aggregate initialization" diagnostic is weirdly apologetic

opened 05:32AM - 08 Mar 23 UTC

good first issue clang:diagnostics

See: https://godbolt.org/z/xKW4EP7zP The full text of this diagnostic is: …"sorry, lifetime extension of temporary created by aggregate initialization using default member initializer is not supported; lifetime of temporary will end at the end of the full-expression [-Wdangling]" Why the "sorry"? This sounds weird and is not in line with other diagnostics. I think this can be improved by simply rewording the diagnostic.

and this is the branch on my fork containing the changes :-

I want to push these changes to llvm-repository, and needed help on the correct procedure since Pull Requests are not accepted by llvm-project.

I can work with git, and need help with the correct procedure to submit patches.

tschuett · March 13, 2023, 8:09pm

Please have a look here.

AryanGodara · March 13, 2023, 8:41pm

Thank you @tschuett , sorry for late reply. I’m new to this platform, and don’t have notifications.
Also, can you please tell me if I’m allowed to make this contribution (as an outreach applicant) or do I need special permissions to submit contributions to llvm?

Thanks!

tschuett · March 13, 2023, 8:51pm

Anybody can open a review request. You have to ask somebody to commit your patch to the mono repo.

AryanGodara · March 13, 2023, 8:53pm

I see, thanks, I’ll go through the steps and update here once it’s done!

AryanGodara · March 13, 2023, 9:50pm

@tschuett , I’m setting up arc (and I’m facing issues, as this is more complicated than simple git😅)
I’m stuck at this step (in arc quick config guide)

$ cd yourproject/
yourproject/ $ $EDITOR .arcconfig
yourproject/ $ cat .arcconfig
{
  "phabricator.uri" : "https://phabricator.example.com/"
}
Set phabricator.uri to the URI for your Phabricator install (where arc should send changes to).

This is the .arcconfig of the clone of my fork of llvm-project (on my local ubuntu machine) :-

{
  "phabricator.uri" : "https://reviews.llvm.org/",
  "repository.callsign" : "G",
  "conduit_uri" : "https://reviews.llvm.org/",
  "base": "git:HEAD^",
  "arc.land.onto.default": "main",
  "arc.land.onto": ["main"]
}

What should I change the URI to?

And finally, I get this error

arc install-certificate                                                                                                                      ─╯
CONFIGURATION ERROR

You need to install the cURL PHP extension, maybe with 'apt-get install php5-curl' or 'yum install php53-curl' or something similar.

But both apt and apt-get don’t have php5-curl

sudo apt install php5-curl                                                                                                                   ─╯
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
Package php5-curl is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source

E: Package 'php5-curl' has no installation candidate

(I had already install PHP previously, using sudo apt install php

Sorry for the complicated message, but I’m really lost!

smeenai · March 13, 2023, 10:34pm

LLVM’s .arcconfig is already set up; you don’t need to make any changes to it

I don’t use Ubuntu myself, but I think the problem is the specific version number in the apt command. Does sudo apt install php-curl work?

AryanGodara · March 14, 2023, 7:12am

@smeenai , Thanks! this worked, and I Completed the install-certificates step.
Now, when I run arc diff, I get linting errors in this piece of code, which is already merged 6 months ago, and not changed by me :-

arc lint                                                                                                                                         ─╯
 Exception 
Some linters failed:
    - CommandException: Command failed with error #1!
      COMMAND
      bash utils/arcanist/clang-format.sh clang/unittests/Format/FormatTest.cpp
      
      STDOUT
      (empty)
      
      STDERR
      /home/aryan/llvm-project/clang/unittests/Format/.clang-format:2:1: error: unknown key 'InsertBraces'
      InsertBraces: true
      ^~~~~~~~~~~~
      Error reading /home/aryan/llvm-project/clang/unittests/Format/.clang-format: Invalid argument
      
(Run with `--trace` for a full exception trace.)

Also, how do I push these changes for review? arc land (main, origin/main, local-branch-name, upstream/main → none of these work)

My steps were :-

Fork llvm-project
checkout new-branch
make changes locally
git add .
git commit -s -S -m “initial commit”
set up arc
arc diff (gives the linting error)
arc land (can’t figure this out yet)

mehdi_amini · March 14, 2023, 9:29am

The error points at a config that is unsupported. This says that your installation of clang-format is likely too old.

AryanGodara · March 14, 2023, 11:09am

clang-format --version                                                ─╯
Homebrew clang-format version 13.0.1

What version should I upgrade this to? (Also, is there specific command to upgrade just clang-format, or do I upgrade everything together?)

mehdi_amini · March 14, 2023, 12:56pm

What version should I upgrade this to?

Latest (15) I think?

(Also, is there specific command to upgrade just clang-format, or do I upgrade everything together?)

This is specific to your system, you could build the clang-format binary from source and add it to your path for example.
For home-brew it should be enough to brew install clang-format ?

AryanGodara · March 14, 2023, 1:00pm

I tried that by trial and error brew install clang-format, it somehow set homebrew to autoupdate all the brew apps, and took half hr haha, but it finally upgraded llvm, and I have clang-format version 15.0.7 now.

So, should I again run these commands?

arc diff
arc land

tobiashieta · March 14, 2023, 1:01pm

arc diff posts the diff for review, when that’s accepted, you need to run arc land to push it to github.

mehdi_amini · March 14, 2023, 1:07pm

If you don’t have commit access, don’t try arc land. When a revision is accepted, ask in the comments for the reviewer to land it for you (and mention your author information: name/email to associate with the commit)

AryanGodara · March 14, 2023, 1:16pm

@tobiashieta @mehdi_amini
Sorry to disturb again, but I’m still getting the same error using arc diff

arc diff                                                                                                                                     ─╯
Launching editor "editor"...
Provide the details for a new revision, then save and exit.
Linting...
 Exception 
Some linters failed:
    - CommandException: Command failed with error #1!
      COMMAND
      bash utils/arcanist/clang-format.sh clang/unittests/Format/FormatTest.cpp
      
      STDOUT
      (empty)
      
      STDERR
      /home/aryan/llvm-project/clang/unittests/Format/.clang-format:3:1: error: unknown key 'LineEnding'
      LineEnding: LF
      ^~~~~~~~~~
      Error reading /home/aryan/llvm-project/clang/unittests/Format/.clang-format: Invalid argument
      
(Run with `--trace` for a full exception trace.)

This file or even that general section wasn’t modified by me, so is there a way to prevent lint check before pushing the code for review?

mehdi_amini · March 14, 2023, 1:20pm

I’m surprised: the linter shouldn’t touch directories you haven’t touched!

--nolint?

@owenpan : seems like you added this, but this isn’t supported by the latest clang-format?

AryanGodara · March 14, 2023, 1:26pm

It worked (I hope so), this is the output :-
And I love the british-spelling log, makes the llvm less intimidating

arc diff --no-lint                                                                                                                           ─╯
(Assuming '--no-lint' is the British spelling of '--nolint'.)
Launching editor "editor"...
Provide the details for a new revision, then save and exit.
Running unit tests...
No unit test engine is configured for this project.
 SKIP STAGING  No staging area is configured for this repository.
Updating commit message...
Created a new Differential revision:
        Revision URI: https://reviews.llvm.org/D146041

Included changes:
  M       clang/include/clang/Basic/DiagnosticCommonKinds.td
  M       clang/include/clang/Basic/DiagnosticSemaKinds.td
  M       clang/lib/Sema/SemaDeclAttr.cpp
  M       clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  M       clang/test/CXX/drs/dr16xx.cpp
  M       clang/test/Lexer/SourceLocationsOverflow.c
  M       clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
  M       clang/unittests/Format/FormatTest.cpp
  M       clang/www/demo/index.cgi
  M       lldb/examples/synthetic/libcxx.py
  M       llvm/include/llvm/CodeGen/MachinePassManager.h
  M       llvm/tools/bugpoint/ExecutionDriver.cpp
  M       llvm/tools/bugpoint/ExtractFunction.cpp
  M       llvm/utils/check-each-file
  M       polly/lib/External/isl/imath/tests/test.sh

AryanGodara · March 14, 2023, 1:28pm

What should be the next step from here?
Is it finding a reviewer? (I just got a mail from Phabricator regarding the commit)
My outreach mentor is @SamTebbs33 !

mehdi_amini · March 14, 2023, 1:29pm

Seems that you actually modified a file there, which makes the linter behavior as expected.

AryanGodara · March 14, 2023, 1:46pm

I see, sorry for missing it
I’ll try and fix this asap